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

Custom spacing without "full" does not merge while "auto" does #377

Open
WesselKroos opened this issue Feb 9, 2024 · 4 comments
Open

Custom spacing without "full" does not merge while "auto" does #377

WesselKroos opened this issue Feb 9, 2024 · 4 comments
Labels
bug Something isn't working context-v2 Related to tailwind-merge v2

Comments

@WesselKroos
Copy link

Describe the bug

Our tailwind config has a custom spacing without the Tailwind default "full" and "auto" spacings. But the "auto" spacing is correctly merged while the "full" spacing is not.

twMerge(...) Results:

w-full w-auto = w-full w-auto
w-8 w-16 = w-16
w-8 + w-16 = w-16
w-full + w-16 = w-full w-16
w-8 + w-full = w-8 w-full
w-8 + w-auto = w-auto
w-auto + w-16 = w-16
w-auto + w-full = w-auto w-full
w-full + w-auto = w-full w-auto
h-8 + h-16 = h-16
h-full + h-16 = h-full h-16
h-8 + h-full = h-8 h-full
h-8 + h-auto = h-auto
h-auto + h-16 = h-16
h-auto + h-full = h-auto h-full
h-full + h-auto = h-full h-auto
h-8 + h-16 = h-16
h-full + h-16 = h-16
h-8 + h-full = h-full
h-8 + h-auto = h-auto
h-auto + h-16 = h-16
h-auto + h-full = h-full
h-full + h-auto = h-auto

To Reproduce

tailwind.config.js example:

export default {
   ...,
  spacing: {
    8: '.8rem',
    16: '.16rem'
  }
}

Merge code example:

const twMerge = extendTailwindMerge({
    "override":{
        "theme":{
            "spacing":[
                "8","16",
                // // Remove the comment below to fix the bug
                // "full"
            ],
        }
    }
});
twMerge('w-8', 'w-full'); // result: w-8 w-full
twMerge('w-auto', 'w-full'); // result: w-auto w-full

Steps to reproduce the behavior

https://codesandbox.io/p/sandbox/hungry-wu-636mq5?file=%2Fsrc%2Findex.ts

Expected behavior

I would expect the tailwind default auto and full spacings, that are both still available with a custom config, to behave the same in twMerge.

Environment

Windows 11, Node 20.10.0

  • tailwind-merge version: 2.2.1

Additional context

We have temporarily fixed this bug by manually adding the full spacing to our extendTailwindMerge config.

@dcastil dcastil added bug Something isn't working context-v2 Related to tailwind-merge v2 labels Feb 12, 2024
@dcastil
Copy link
Owner

dcastil commented Feb 12, 2024

Hey @WesselKroos! 👋

I could reproduce it and indeed the issue is that tailwind-merge includes the full value as part of the spacing scale (code) while Tailwind CSS defines it as part of the individual scales like width (code).

I have to think about how to best fix this. The big question is whether tailwind-merge should favor being as close as possible to the Tailwind CSS config or prefer a smaller bundle size.

Removing the full value from the spacing scale in tailwind-merge and move it to all the individual scales that use it would make the already quite large config even bigger than it is. And I'm not sure whether that would make sense overall.

@WesselKroos
Copy link
Author

WesselKroos commented Feb 12, 2024

Thanks for the explanation and the code links.
Then that means that it also happens for the values screen, 1/2, 1/3, 2/3, 1/4, 2/4, 3/4, 1/5, 2/5, 3/5, 4/5, 1/6, 2/6, 3/6, 4/6, 5/6, 1/12, 2/12, 3/12, 4/12, 5/12, 6/12, 7/12, 8/12, 9/12, 10/12, 11/12. As far as I'm able to deduct from the Tailwind configuration.

I understand what you mean by "it would make the already quite large config even bigger than it is".
Tailwind only generates the classes you use in a project via the content configuration option. Do you think it could make sense to use a similar approach here? / Would it be worth to investigate if we can hook into Tailwinds content deduction logic some way to generate a minimal runtime config for tailwind-merge?

Or alternatively,
We are currently parsing our tailwind config to tailwind-merge with a few converter functions. I suppose we could take tailwind's default config, merge it with our config and then pass it to tailwind-merge. So that tailwind's defaults are also included in our tailwind-merge config. I guess that would also resolve this issue for our project, but that would mean that our tailwind-merge config becomes large. So I would need to double check if our bundle size is still ok in that situation.

Or thinking even simpler,
Would it make sense to always merge tailwindcss class prefixes that do not conflict with multiple css properties independent from the config? Like those that start with w-, h-, min-w-, max-w-, min-h- and max-h-? (I suspect there is a reason why it's currently not done that way.)

@dcastil
Copy link
Owner

dcastil commented Feb 12, 2024

Oh yeah, you're right, the fractions have the same problem.

Do you think it could make sense to use a similar approach here? / Would it be worth to investigate if we can hook into Tailwinds content deduction logic some way to generate a minimal runtime config for tailwind-merge?

Yes, that idea is already floating around for some time. The problem with this is that I'd need to know from Tailwind which classes it produced. I asked for an API for that in tailwindlabs/tailwindcss#10348 but it's not possible to say if and when that would be built.

I suppose we could take tailwind's default config, merge it with our config and then pass it to tailwind-merge.

The full Tailwind CSS config is really big unfortunately, bigger than tailwind-merge itself. It's also not so easy to pass a Tailwind config to tailwind-merge because you'd need to pass scale values around manually quite often – tailwind-merge only allows to use a subset of all theme keys, the rest must be modified in the classGroups object manually.

I don't recommend to go this route.

Would it make sense to always merge tailwindcss class prefixes that do not conflict with multiple css properties independent from the config?

That is sort of happening already, but also not at the same time. tailwind-merge merges all values that look like they could be part of the default scale. E.g. twMerge('w-2 w-123456789') === 'w-123456789' although w-123456789 isn't part of the default Tailwind CSS config. You can use any w-<number> class and tailwind-merge would detect it as a width class. This feature makes it easy to do small changes to your Tailwind config without needing to configure tailwind-merge. But tailwind-merge won't detect w-nope as a width class because that could be a non-Tailwind class which tailwind-merge needs to pass through unchanged.

The way to go with tailwind-merge with the least configuration issues is to configure Tailwind as close as possible to the default, meaning that you keep using numbers for number scales, T-Shirt sizes fort T-Shirt scales (you can use 2xs, 3.5xl to extend them), etc. (see here). Then you don't need to configure those scales in tailwind-merge at all. You only have to add the few places where you can't get around configuring tailwind-merge, e.g. when adding additional animations, but that would ideally be just a few cases.

This is far from ideal, but probably the least painful way to do it until I figure out how to build the config automatically.

@WesselKroos
Copy link
Author

Yes, #185 is already floating around for some time.

That's great to hear. Let's hope that they implement such API soon.

The full Tailwind CSS config is really big unfortunately, bigger than tailwind-merge itself. It's also not so easy to pass a Tailwind config to tailwind-merge because you'd need to pass scale values around manually quite often – tailwind-merge only allows to use a subset of all theme keys, the rest must be modified in the classGroups object manually.

That's unfortunate, then it makes sense that we will go the classGroups route for now.

Thanks for your time, insights and the work on tailwind-merge!

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

No branches or pull requests

2 participants