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 config option to ignore variants/modifiers #440

Closed
wants to merge 1 commit into from

Conversation

barvian
Copy link
Contributor

@barvian barvian commented Jun 29, 2024

Hi! Thanks a lot for the library! Sorry to submit a PR out of the blue, but it was so small that I thought starting a discussion first would be overkill.

I'm trying to use tailwind-merge with a third-party Tailwind plugin I created but I'm running into some issues. Unlike most Tailwind variants, the variants in my plugin change a utility's properties, not its selector (I've seen this in other plugins as well). Currently, these types of variants don't work well with tailwind-merge, because it (reasonably) treats them as normal variants when ideally they would be ignored.

As a potential solution, I added an ignoredVariants array (default: []) to the config for variants that should be ignored when merging classes. Do you think you'd be interested in adding a feature like this? I'd love to improve my plugin's compatibility with tailwind-merge (my users would appreciate it as well 😅)

Thanks!

@dcastil
Copy link
Owner

dcastil commented Jul 1, 2024

Hey @barvian! 👋

Thanks for your PR, I appreciate that a lot. Also congrats on your plugin, that looks cool!

I try to keep tailwind-merge focused on the default use cases when using vanilla Tailwind CSS with a few changes to the config to keep the library simple, fast and flexible enough for future features of Tailwind CSS. So I'm afraid it wouldn't fit very well to add the ignoredVariants property to the config.

I see that this doesn't work with your plugin and there were also requests for other more advanced use cases. I want to expose a lower-level API so that tailwind-merge works with all those use cases in #385 (just added this PR as an example there).

Would it be helpful to you if I instead create this API that allows you to deeply customize the behavior of tailwind-merge to work with your plugin?

It probably wouldn't be a config but rather a new function that returns an object with a bunch of low-level util functions (including the twMerge function). This means you'd need to release this twMerge (or some form of extendTailwindMerge yourself or tell your users how to set up this lower level function.

Copy link
Owner

@dcastil dcastil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two parts in this PR that we can merge though, if you want to isolate them. Thanks for fixing those!

docs/configuration.md Show resolved Hide resolved
src/lib/types.ts Show resolved Hide resolved
@barvian
Copy link
Contributor Author

barvian commented Jul 1, 2024

Thanks for the review! I totally understand not wanting to support this feature. As for the new API, I'm sure it'd be useful! I'll close this PR and create a new one for the typo/comment fixes 🙂

@barvian barvian closed this Jul 1, 2024
@barvian barvian mentioned this pull request Jul 1, 2024
@dcastil
Copy link
Owner

dcastil commented Jul 2, 2024

Alright, I'll take your use case into account and let you know once it is done.

@dcastil
Copy link
Owner

dcastil commented Jul 7, 2024

@barvian just merged #444 which adds an experimentalParseClassName property to the config. There is some documentation for it in https:/dcastil/tailwind-merge/pull/444/files#diff-b8b77f5be18cf56dca425b3a5b8e9d2e754dd37fe0e3612b95cd4e9bccda08a5.

You should be able to use that to build a tailwind-merge plugin in case you want to build it. But be aware that the API might change in a minor update. I see this as a smaller stepping stone on the way to getting to a separate API that allows even more stuff.

@barvian
Copy link
Contributor Author

barvian commented Jul 7, 2024

@dcastil Amazing, I'll play around with this! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context-v2 Related to tailwind-merge v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants