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

Next version of tailwindcss could have `text-[start,end]` built-in #33

Closed sa3dany closed 2 years ago

sa3dany commented 2 years ago

See: https://github.com/tailwindlabs/tailwindcss/pull/6656

Just in case. Will happily create a pull request after the new version ships with these utility classes built-in.

stevecochrane commented 2 years ago

Hi @sa3dany, thanks for letting me know about this! tailwindcss-logical already supports those (and uses the exact same class names used in the above Tailwind PR) so there’s no need for a PR here, but thanks again for the kind offer. That said, I’m glad you brought this up because I think this is the first time that something from tailwindcss-logical will be generated from Tailwind itself, so I’ll want to do some quick tests to make sure nothing breaks.

sa3dany commented 2 years ago

I was actually thinking the PR will be removing the text-start and text-end from the plugin. I thought that they might conflict with the classes from tailwindcss itself.

stevecochrane commented 2 years ago

Oh, sorry for misunderstanding. When I first saw this my thought was “since the classes are exactly the same, maybe I’ll keep things as-is to avoid a version bump” but after some more thought, I agree that it would be best to remove features from this plugin as they’re integrated with Tailwind itself. We’ll need to do a major version bump on the plugin each time, since it’s a breaking change if someone isn’t using the latest version of Tailwind, but I think it’s the right thing to do.

So, if you would like to submit a PR to remove text-start/text-end when the next version of Tailwind goes live, that would be greatly appreciated! And it doesn’t have to be comprehensive; any contributions are appreciated and then if there are tests, documentation, etc. that still need to be updated, I will gladly handle the rest. I’ll also be glad to credit you for your contributions to the project.

sa3dany commented 2 years ago

Hi @stevecochrane, I did a small-scale test where I added the following to my tailwind.config.js:

plugins: [
    plugin(function ({ addUtilities }) {
      addUtilities({
        ".text-right": {
          "text-align": "right",
        },
      });
    }),
],

The result was duplicate css rules for .text-right. Testing with a different css declaration e.g foo: bar, resulted in both the declarations appearing in the un-minified css output, the original built-in tailwind declaration(s) plus the plugin declaration(s).

Building in production with minification, results in only one declaration in the output (in the case of plugin adding the same declaration as the built-in tailwind utility).

So for now, I think it's safe to leave the plugin as-is since minification in production takes care of any duplicate rules.

Edit: And feel free to close the issue in that case if you agree 👍🏼

stevecochrane commented 2 years ago

I see, thank you for running that test. I’m trying to think of other cases where having a duplicate might break things, but I can’t think of anything, so it does seem safe to me too. Rather than doing a major release just to remove this when there’s no real benefit to it, it does seem best to hold off until we have a good reason to do a major release, and then consider if we should deprecate this at the same time.

And in the future if Tailwind CSS does integrate something that tailwindcss-logical provides but their implementation differs from ours, then we’ll definitely want to deprecate that from the plugin right away to prevent anything from breaking.

In that case I think I will close this issue. Thank you again for your help with this!