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

dark variant not respecting bg-opacity #2966

Closed
Yiddishe-Kop opened this issue Dec 1, 2020 · 7 comments
Closed

dark variant not respecting bg-opacity #2966

Yiddishe-Kop opened this issue Dec 1, 2020 · 7 comments

Comments

@Yiddishe-Kop
Copy link
Contributor

Yiddishe-Kop commented Dec 1, 2020

Describe the problem:

I have darkMode: 'class' in my config.

When using these classes: bg-gray-100 bg-opacity-50 dark:bg-gray-900 , the bg-opacity is only applied in light mode, but in dark mode bg-opacity is ignored.

I had to add the dark variant to: backgroundOpacity: ['dark'], then also add dark:bg-opacity-50 to the element to get it to work in dark mode.

Expected behaviour: dark mode should also respect the bg-opacity class (unless overriden for dark).

Link to a minimal reproduction:

https://play.tailwindcss.com/GHoILXz6cp

@Yiddishe-Kop
Copy link
Contributor Author

I've narrowed down the issue:

bg-opacity-40 is correctly setting --tw-bg-opacity to 0.4, but dark:bg-gray-900 is also overriding --tw-bg-opacity back to the default 1.

See screenshot:
CleanShot 2020-12-01 at 3 24 09

I guess the order needs to be changed, so that bg-opacity-40 shouldn't be overriden by dark:bg-gray-900.

@adamwathan
Copy link
Member

Yeah this is currently by design for better or for worse, and is the same with media queries too like:

bg-blue-500 bg-opacity-50 md:bg-red-500

That won't preserve the opacity either. I can see arguments for both behaviors but unfortunately we likely can't change this as it would be a breaking change.

We should probably enable dark by default for the opacity utilities though, would accept a PR for that 👍🏻

@marceloadsj
Copy link

marceloadsj commented Dec 1, 2020

Couldn't the:

.dark .dark\:bg-gray-900 {
    --tw-bg-opacity: 1;
    background-color: rgba(17, 24, 39, var(--tw-bg-opacity));
}

Be like this, instead?

.dark .dark\:bg-gray-900 {
    background-color: rgba(17, 24, 39, var(--tw-bg-opacity, 1));
}

I'm not seeing any downside, but I don't have a big picture to think off right now.
But, yes, I see the breaking change that this can lead to. Maybe a flag on config?

@marceloadsj
Copy link

Ps: this is a bit related to this one I opened couple of days ago: #2946
Both behaviours generated by having the variable/custom prop set together with an attribute, instead of using an extra var() argument.

@DylanVann
Copy link
Contributor

@marceloadsj I think the problem would be the cascade. If tw-bg-opacity is set above then it will affect your color, which people likely would not consider or want when they use just a color class on something.

An alternative way to implement dark mode that doesn't have this issue is to use CSS variables for your color palette. I have a demo of this: https://github.com/DylanVann/tailwind-css-variable-colors-demo

The downside I see of this is that the color palette is a bit verbose to define and is not purged.

@marceloadsj
Copy link

You are right @DylanVann. Since the variable would not be directly into the color class, it could be at any place above the tree, and affect the color. I didn't think about it!

@adamwathan
Copy link
Member

Looking into this further this is actually a little more annoying than it looks — it turns out bg-opacity-50 for example will apply on top of dark:bg-white or whatever if you use the media dark mode, but if you use the class dark mode it won't. This is actually because of the selector specificity, not because of the order. .dark .dark\:bg-white has a higher specificity than bg-opacity-50.

This means that currently the behaviors are different depending on which dark mode you use, and that really sucks but I can't think of a way to avoid it.

One approach would be to change the color plugins to work the way box shadow does where we add a universal selector to set the initial opacity, to avoid the cascade problems @DylanVann explained:

* {
  --tw-bg-opacity: 1
}

But this would be a breaking change, because this code would behave differently:

<div class="bg-red-500 bg-opacity-50 md:bg-blue-500">

Currently that code would have a 100% opacity blue 500 background on md screens, but with this change it would preserve the opacity set by bg-opacity-50 on the other screens. This is honestly probably a better behavior but it's breaking, so not something we can change until v3.

I've merged the PR to enable dark variants for for color opacity utilities regardless since that will help in this situation but I don't think we can do anything about the consistency between media and class modes for at least a year or two I'm afraid :/

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

4 participants