rails / tailwindcss-rails

Other
1.39k stars 170 forks source link

Remove Inter font assets from inclusion in downstream projects #301

Closed stefanlindbohm closed 4 months ago

stefanlindbohm commented 8 months ago

This gem currently bundles the Inter font as part of its assets, which gets included in any project depending on it. As such it will also be picked up by rails assets:prepare and output into the public assets directory, whether the project uses the font or not. I'd like to propose these assets be removed from tailwindcss-rails or alternatively be included in a way that does not force assets into downstream projects.

I know Inter is often used in Tailwind examples and as a starting point for smaller projects. This makes sense. But this gem should in my mind also be the preferred way for larger Rails projects using Tailwind, where Inter is unlikely to be the font of choice. In these cases, including the font assets with no way of opting out creates unnecessary clutter in those projects & deployments.

Possible actions

  1. Remove the font assets. The Inter font project has an official CDN provided by Cloudfront that could be a documented suggestion for new projects & upgrade path. This would of course be a breaking change.
  2. If there's a way for Rails engines to conditionally provide assets, move to using that. I'm not aware of any such methods myself though(?).

Workaround

I'm currently hacking my way out of these assets with the following configuration. I'm open to suggestions on how to do this in a cleaner way if possible. A clean & documented way to do this might remove the need for the proposed change.

Rails.application.config.to_prepare do
  Rails.application.config.assets.precompile -= %w[inter-font.css]
  Rails.application.config.assets.paths.reject! { _1.is_a?(String) && _1.include?('gems/tailwindcss-rails') }
end

I'd like to hear what you (maintainers) think about this. If this is a desirable change, I'm open to working on a PR.

robzolkos commented 8 months ago

I've been adding the workaround snippet above to a bunch of my projects to mitigate Inter being included.

This has become more of an issue since the :all option was added to stylesheet_link_tag in Propshaft in this PR

The inter.css file will be downloaded by the browser, even if there is no Inter font usage in the app.

imo - as Inter is not a part of Tailwind per se - it should not be a part of this gem. That's probably a separate discussion. Perhaps it could be an optional inclusion.

flavorjones commented 8 months ago

@stefanlindbohm I think we're open to a PR to improve this.

Just to be explicit about the goals I think we're agreeing on:

The naive solution that comes to mind for me is to move the asset initialization from

https://github.com/rails/tailwindcss-rails/blob/d52b49e6dbd08eae2ac7afb1f2249a10cbd557f4/lib/tailwindcss/engine.rb#L5-L7

into an app initializer that is generated during tailwindcss:install.

The only tricky part would be making the gem backwards-compatible with apps that don't have that initializer installed.

WDYT? What ideas do you have?

dixpac commented 8 months ago

I would remove inter font, maybe use system-ui or just leave default one which is font-family: ui-sans-serif, system-ui, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; (afaik)

robzolkos commented 8 months ago

I would remove inter font

I agree. Here is a diff of the scaffolded views with and without Inter (without uses the default Tailwind font stack). It looks great without Inter!

comparison

The only tricky part would be making the gem backwards-compatible with apps that don't have that initializer installed.

True but maybe a major version bump and an education piece on how to add it back (or a link to add fonts in general) may work?

stefanlindbohm commented 8 months ago

Thanks for the responses, everyone.

@flavorjones:

Just to be explicit about the goals I think we're agreeing on:

  • we should keep the "freshly installed" experience by shipping a good font and styling out of the box
  • once developers start customizing their styling and assets, it should be easy to remove the default "inter-font.css"

Sounds reasonable to me. I would personally add the goal that minimal complexity should be carried as part of this gem, with the assumption that the default font is just a starting point that will be customized by many/most projects. The point @robzolkos makes that Inter is not part of Tailwind per se is relevant here.

The only tricky part would be making the gem backwards-compatible with apps that don't have that initializer installed.

Right. Whether we can do breaking changes (and a major version bump) decides what paths we can take.

Steps I can see for a non-breaking change:

A breaking change could be done by:

I would vote for doing the breaking change as that would simplify things as well as making these defaults explicit in newly setup apps (I suspect that many apps might already be using other fonts & are unknowingly deploying/loading unused assets). What do you think about this?

dixpac commented 8 months ago

I think could make it work without the "breaking change", but I agree with @robzolkos and @stefanlindbohm . Some day this "baggage" will have to be dropped sooner or latter.