Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add line-height modifier support to font-size utilities (introduced in Tailwind CSS v3.3) #211

Closed
dcastil opened this issue Mar 28, 2023 · 5 comments · Fixed by #214
Closed
Labels
context-v1 Related to tailwind-merge v1 feature request

Comments

@dcastil
Copy link
Owner

dcastil commented Mar 28, 2023

This issue is about adding support for the line-height modifier introduced in Tailwind CSS v3.3 (announcement, pull request).

I separated that feature into a separate issue from the Tailwind v3.3 support PR because the dynamic nature of the modifier makes it a bit tricky to implement in tailwind-merge.

Tailwind allows to use / in Tailwind classes, e.g. you could use the color name red/10 which would produce classes like text-red/10 (the 10 is not a modifier in this case). This makes the postfix modifier very difficult to detect. tailwind-merge can't know whether the / character marks the transition to a postfix modifier or whether it's just part of the name (at least in the current default config with using validators.isAny for colors).

Moreover, with the implementation of a generalized postfix modifier in tailwindlabs/tailwindcss#9541 I think we might see more postfix modifiers out there, so it makes sense to think about a generalized solution for tailwind-merge to prevent more breaking changes in the future.

I see several paths forward:

1. Every / is a modifier (see #211 (comment))

Detect something that could be a potential postfix modifier splitModifiers and remove it from the baseClassName.

Pros:

  1. Universal solution that will work for any modifier without any config.

Cons:

  1. We're searching every single class for a modifier although only few will have one which is wasteful during runtime. But probably not a huge issue for perf.
  2. tailwind-merge can't support the / character in classes outside of arbitrary values ([<here>]) since it will assume it is a modifier. This would be a breaking change in tailwind-merge. The question here is how many people use / in their Tailwind theme names.

2. Every / can be a modifier

Same like 1 but if we don't find a match for the base class with modifier, we perform another search for the base class without modifier as it could be part of a name.

Pros:

  1. Universal solution that will work for any modifier without any config.
  2. No breaking change

Cons:

  1. We're searching every single class for a modifier although only few will have one which is wasteful during runtime. But probably not a huge issue for perf.
  2. We will perform 2 searches for every class that has / in their name which has runtime cost.

3. Config for which classes have modifier

Add the ability to specify which classes have modifiers to the tailwind-merge config. This could be implemented in several levels of precision vs. brevity.

Pros:

  1. Computations only applied to classes that actually support postfix modifiers which is better for perf.
  2. No breaking change unless someone customized their config heavily.

Cons:

  1. Puts burden on user to configure
  2. Might be more complex/error prone in implementation

Other

@dcastil dcastil added help wanted Extra attention is needed feature request labels Mar 28, 2023
@dcastil dcastil pinned this issue Mar 28, 2023
@dcastil
Copy link
Owner Author

dcastil commented Mar 28, 2023

@adamwathan Hope it's okay to reach out to you here. Do you think it's reasonable to expect that people don't use / within names in Tailwind configs and that it basically only appears as the modifier indicator? Have you seen people put / in their theme keys?

@adamwathan
Copy link

@dcastil Hey! So people do use / in there, we even do it in core for things like w-1/2 for example. They probably don't for font sizes but we've tried to implement our handling around this stuff in Tailwind in a generic way, so that if we see text-5/6 in a template and there's a font size called 5/6 we use that instead of 5 with a 6 line-height if that makes sense. Edge case as fuck though for sure.

@dcastil
Copy link
Owner Author

dcastil commented Mar 28, 2023

Ah yes, forgot about those. 😅

Okay that makes a lot of sense! Then I'll replicate this behavior by first looking for 5/6 and if that is a miss, look for 5.

Thanks for your input!

@dcastil dcastil removed the help wanted Extra attention is needed label Mar 29, 2023
@dcastil dcastil unpinned this issue Mar 29, 2023
@dcastil
Copy link
Owner Author

dcastil commented Apr 1, 2023

Note to myself: Going with option 2, first searching for full class name and if that is unsuccessful, search for class name without potential modifier first searching for class name without potential modifier and if that is unsuccessful, search for full class name (see #215).

  • Has least impact on bundle size
  • No config burden on user
  • Performance impact is negligible
  • Incorrect merges very unlikely

@github-actions
Copy link

github-actions bot commented Apr 5, 2023

This was addressed in release v1.12.0.

@dcastil dcastil added the context-v1 Related to tailwind-merge v1 label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context-v1 Related to tailwind-merge v1 feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants