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

Check for class group with modifier first before checking without modifier #215

Closed
dcastil opened this issue Apr 2, 2023 · 3 comments
Closed
Labels
breaking Is breaking change context-v2 Related to tailwind-merge v2 feature request
Milestone

Comments

@dcastil
Copy link
Owner

dcastil commented Apr 2, 2023

Tailwind CSS first checks for class group with potential postfix modifier included (source), so tailwind-merge should do that too to prevent diverging behavior between the two libraries.

I couldn't do that in #214 because that would have been a breaking change. The reason is that if we first check for groups with the modifier included, a class like text-lg/7 which belongs into the font-size group will be incorrectly identified as a text-color group since colors use the catchall validator isAny.

So to be able to first check for the group with the modifier, the general use of isAny needs to go, meaning that tailwind-merge will be more strict about which colors it accepts in its default config. Hence the breaking change.

I could imagine to use following validators for colors.

function isColor(value: string) {
    return /^([a-z]+-)+\d+$/.test(value)
}

function isArbitraryColor(value: string) {
    return getIsArbitraryValue(value, 'color', isAny)
}

I should keep in mind that this feature would bring me further away from a potential minimal tailwind-merge config which would rely on the isAny validator a lot. Therefore I'm not 100% sure whether I want to do this change.

@dcastil dcastil added breaking Is breaking change feature request labels Apr 2, 2023
@dcastil dcastil added this to the v2 milestone Apr 2, 2023
@dcastil
Copy link
Owner Author

dcastil commented Apr 2, 2023

Commit where lookup with and without modifier was switched: 9b35ce5

@dcastil
Copy link
Owner Author

dcastil commented Aug 12, 2023

I'm less sure that it makes sense to implement this change. A lot of users would need to modify their color config as a result of this and if a color like white, inherit or transparent would be added, it's easy to forget to add that to the isColor function. I can also imagine that a lot of people would just add back the isAny validator which then would cause the postfix modifier behavior to work unexpectedly.

All in all the current behavior is probably more safe in terms of unintended consequences when configuring tailwind-merge.

@dcastil
Copy link
Owner Author

dcastil commented Aug 19, 2023

I won't do this in v2 due to reasons explained in #215 (comment).

@dcastil dcastil closed this as not planned Won't fix, can't repro, duplicate, stale Aug 19, 2023
@dcastil dcastil added the context-v2 Related to tailwind-merge v2 label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Is breaking change context-v2 Related to tailwind-merge v2 feature request
Projects
None yet
Development

No branches or pull requests

1 participant