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

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

Closed
sa3dany opened this issue Mar 28, 2022 · 5 comments
Closed

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

sa3dany opened this issue Mar 28, 2022 · 5 comments

Comments

@sa3dany
Copy link
Contributor

sa3dany commented Mar 28, 2022

See: tailwindlabs/tailwindcss#6656

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

@stevecochrane
Copy link
Owner

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
Copy link
Contributor Author

sa3dany commented Mar 29, 2022

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
Copy link
Owner

stevecochrane commented Mar 31, 2022

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
Copy link
Contributor Author

sa3dany commented Apr 2, 2022

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
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants