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 back fill-current and stroke-current #6357

Closed
wants to merge 1 commit into from
Closed

Add back fill-current and stroke-current #6357

wants to merge 1 commit into from

Conversation

dcastil
Copy link
Contributor

@dcastil dcastil commented Dec 10, 2021

I created a PR instead of an issue due to the small change. Feel free to close if undesired or to keep it as a space for discussion if unsure. 😊


I noticed that the fill-current and stroke-current utilities were removed in #5933.

Despite the full color palette being available for the fill and stroke properties, I think it's important to keep the -current versions because every single design system I worked on uses those values for icons meant to be placed within buttons, etc. The icons should change their color if the text color is changed. This is especially important in component-based UI frameworks where a className can be passed into a component.

It's also possible to put these into the project-specific Tailwind config but maybe it makes sense to keep those in the default config since they were present before and don't cause a penalty on the runtime if they aren't used.

@RobinMalfait
Copy link
Contributor

Hey! Thank you for your PR!
Much appreciated! 🙏

The fill-current and stroke-current still exist because the value current is included in the colors object:

The only moment this is an issue is if you are overriding the full color object instead of extending it. But now that JIT mode is the default, it doesn't hurt to extend your colors instead of overriding them.

@adamwathan do you think we should add it back for this specific use case?

@adamwathan
Copy link
Member

Hey! These are already in the default config, they are included in the full colors object:

https://github.com/tailwindlabs/tailwindcss/blob/master/stubs/defaultConfig.stub.js#L15

Here's a demo of it working:

https://play.tailwindcss.com/Cc8JbQMhvK

@adamwathan
Copy link
Member

Haha commented same time as @RobinMalfait — I don't think we need to hardcode these back in personally. It works as expected out of the box and if you've overridden your colors and not included current in your custom color palette, I think it's a small ask to suggest people to add current themselves.

Again though this does work out of the box, it only stops working if you explicitly remove current from your palette essentially.

dcastil added a commit to dcastil/tailwind-merge that referenced this pull request Dec 10, 2021
@dcastil
Copy link
Contributor Author

dcastil commented Dec 10, 2021

Oh I missed that it's in the default color theme. I always override the entire color scale with the company-specific scale, so didn't even know that it's there. Thanks for pointing it out to both of you. 😄 I see this as resolved and am closing the PR.

@dcastil dcastil closed this Dec 10, 2021
@dcastil dcastil deleted the add-back-stroke-fill-current branch December 10, 2021 13:51
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

Successfully merging this pull request may close these issues.

None yet

3 participants