Skip to content

Conversation

@bakura10
Copy link
Contributor

Hi,

This PR makes sure that the / 1 is not outputted when there is no opacity defined.

The reason for this change is to allow Tailwind to better play with CSS variables.

For instance, in our use case I have in our config a "current" background defined to a CSS variable:

backgroundColor: ({ theme, rgb }) => ({
  'current': '--background'
})

This --background variable is dynamically set on some div, with optional opacity:

<div class="bg-[rgb(--background)]" style="--background: 255 0 0 / 0.5">
  <span class="text-accent bg-current">0</span>
</div>

The issue is that currently, on the inner span, Tailwind will try to generate this:

.bg-current {
  background-color: rgb(var(--background) / 1);
}

However, due to the fact that --background already contains a component with an alpha, this resolves as rgb(255 0 0 / 0.5 / 1).

This PR allows to add this extra use case :).

@thecrypticace thecrypticace self-assigned this Apr 25, 2022
@thecrypticace
Copy link
Contributor

Hey! So the new rgb and hsl helpers are really designed specifically for the situation where you want to define your colors with CSS variables but still want the opacity modifier to work (bg-current/75). They are not designed or intended to support variables that already have the opacity baked in.

For instance, if --background was set to 255 0 0 / 0.5 and you tried to use bg-current/75, then Tailwind would try to add an alpha channel of 0.75, which would lead to a resolved color of rgb(255 0 0 / 0.5 / 0.75) which is of course invalid.

I don't know all of the details about your exact use case but I could imagine this happening in your project if someone uses bg-current/75 or similar in a component, and then someone defines --background: 255 0 0 / 0.5 on a parent. In your case it sounds like it's very important that nobody ever uses the opacity modifier on the bg-current class because you want people to be able to specify the opacity in the parent (and all that only happens at run-time so you sort of have to treat that as a constraint), so to me, using the rgb helper here is actually not the right solution for you since it's entire reason for existing is to make it possible to use the opacity modifier with CSS variables.

Instead I would recommend just defining the bg-current class without the rgb helper and not worrying about the opacity modifier stuff at all since it simply can't work for your use case anyways:

backgroundColor: {
  'current': 'rgb(var(--background))'
}

This way you have full control over the output and Tailwind won't try to do anything fancy with it on your behalf. Again I think this will be totally fine for your project because you really can't use a class like bg-current/75 in your situation no matter what, since there's no way for the opacity value baked in to your variable to override the /75 modifier at run-time.

It's also important to note that by default, this is the actual CSS that Tailwind will generate for your bg-current class:

.bg-current {
  --tw-bg-opacity: 1;
  background-color: rgb(var(--background) / var(--tw-bg-opacity));
}

...because Tailwind still supports classes like bg-opacity-75 for compatibility with Tailwind v2, even though we now encourage people to use the opacity modifier instead. This means that even with your PR, your example wouldn't work out of the box unless you explicitly disabled the backgroundOpacity plugin to make sure Tailwind doesn't generate anything related to the --tw-bg-opacity variable.

Anyways hope that helps — going to close this because even though there's no real harm in merging it, I don't want to accidentally support something that might tie our hands in the future when we never intended to support it.

Happy to keep discussing this in our discussions area if you think I'm missing something or need some more input on the best way to achieve what you're trying to achieve in your project! Thanks a lot for the PR either way ❤️

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.

2 participants