stevecochrane / tailwindcss-logical

A CSS Logical Properties and Values plugin for Tailwind CSS.
https://stevecochrane.github.io/tailwindcss-logical/
ISC License
172 stars 2 forks source link

Support for tailwind-merge #48

Closed Khaan25 closed 5 months ago

Khaan25 commented 11 months ago

I've this pli-0 class defined on my button ⬇ image

In Button component, I've tailwind-merge, which should remove the old classes if similar ones are present ⬇ image

But it isn't. Any Reason for that? Here's the cn function which does that ⬇ image

Browser Preview

We can see that that pli-0 is not applied due to previously applied classes πŸ™‚ image

stevecochrane commented 11 months ago

Hi @Khaan25, it looks like the β€˜pli-0’ style being overridden is exactly the same as the one that would be generated by tailwindcss-logical, is that correct? (You can see what tailwindcss-logical generates by default in the test cases, and here is the one for β€˜pli-0’ for comparison.) If so, and I’m not too familiar with tailwind-merge, but if you merge two classes with identical styles, does it keep both?

That would be my guess at what might be happening here, but if the styles do have differences, please let me know and I’ll take another look, thanks.

Khaan25 commented 11 months ago

Here's the link: https://www.npmjs.com/package/tailwind-merge https://www.npmjs.com/package/clsx

I think you are right. Tailwind merge haven't added tailwind-logical πŸ™ˆ Any Solutions for this to work?

My solution was to seperate x & y and then do that but it's not optimal...

stevecochrane commented 11 months ago

Hi again @Khaan25, sorry for the late response. I've been on vacation but I'm back now.

If I understand correctly, the goal of tailwind-merge is to merge two lists of class names, and that will not help if there are style conflicts for a single class, which seems to be the issue you're seeing.

For example, let's say I have the following somewhere in my CSS:

.pli-0 { padding-inline: 0; }

And then I have the following somewhere else, later in my CSS:

.pli-0 { padding-inline: 1rem; }

When I use the pli-0 class, I'll see both styles declared in DevTools with the second style overriding the first, just like in your example. That conflict will still be there, whether or not tailwind-merge is used. Both of the following will still result in the pli-0 class being applied, with the same conflict still happening:

className={'pli-0'}
className={twMerge('pis-0 pie-0', 'pli-0')}

Does that make sense? Since there are two pli-0 style rules declared and one is generated from tailwindcss-logical, do you know where the other is coming from?

Khaan25 commented 11 months ago

Yeah. It makes sense. Sooo, What's the solution to this? Shall I downgrade the tailwindcss version v3.3 in which they introduced the logical classes? Will it resolve the issue?

stevecochrane commented 11 months ago

Ah, so the duplicate style rule is coming from Tailwind itself then, eh? You could downgrade your Tailwind version if you really want to remove the duplicate style, but I would not recommend it. The new logical styles provided by Tailwind are identical to what tailwindcss-logical provides, so the only downside is that the user downloads a small amount of unnecessary CSS.

I had a similar discussion like this in a previous issue and we found that in a production build, minification would remove the duplicate styles, so in that case there is no downside at all. Is your example with the duplicate styles running in development mode or production?

When Tailwind first started adding support for logical properties and values, I considered updating tailwindcss-logical to remove any styles that were added, but the problem with that is I would need to make a major release to the plugin every time, since any removal of styles would be a breaking change. It seems too inconvenient to users of the plugin to add multiple new major versions when the duplicate styles have no effect in production, but I could be wrong and I'll keep an open mind if there are further reasons to justify removing the duplicates.

Khaan25 commented 11 months ago

I was using development mode. Maybe you can update your plugin to remove that classes? πŸ™ˆ I really like this logical props, I've made whole website using this. You can checkout here, I don't want to lose this plugin but support it to become a better version πŸš€

Link: https://vennsol.vercel.app/

stevecochrane commented 11 months ago

OK, that's good to know that it's only for development mode.

I've thought about it some more and I think it does make sense to remove the duplicate classes with a new major version release. Previously when Tailwind had just started adding Logical Properties and Values support and had only added a couple text-align values, it made more sense to keep things as-is, but now there's enough overlap to justify the change.

I'll plan on making this change in the near future, but I don't consider it urgent, and it might take me a month or two before I can get around to it. I'll leave this issue open until then. Thank you for your support!

stevecochrane commented 10 months ago

I ran a comparison between Tailwind's Logical Properties classes and tailwindcss-logical's classes, and Tailwind doesn't provide pli-* classes. Their Logical Properties classes for padding are just ps-* (for padding-inline-start) and pe-*(for padding-inline-end). So it isn't possible that Tailwind itself would be applying a pli-* class, and it appears something else is causing pli-* to be applied twice.

There's also very little overlap between Tailwind's naming a tailwindcss-logical's naming in general. The only instance where the class names are exactly the same is for border-radius, so I'm thinking a new major version isn't worth the confusion for users of the plugin and I prefer to keep things as-is.

I'd like to help, but without more detail on what is causing the problem, there's not much I can do on my side. If you're able to point me to something clearly indicating that tailwindcss-logical has a bug, then I'd be glad to take a look, but otherwise I'm unable to investigate. One thing you could try is removing the individual pieces of your CSS workflow one-by-one to see if you can isolate the cause.

Khaan25 commented 10 months ago

Thanks for the support @stevecochrane. I think the problem lies in tailwind-merge cuz they don't support your library classes OR mental modal is different? I had to use !important to fix them and I did cuz I needed to finish the project and wanted this functionality to be there.

If I need any help, I'll ask you in near future 😊

Best Regards,