-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove text opacity CSS variables from ::marker
#8622
Conversation
1c29ae8
to
579b30d
Compare
@RobinMalfait I had to tweak The only knock on effect is that I think this is more likely to correctly rewrite selectors in plugins that add variants that clone rules whereas they wouldn't have been before (… at least I don't think). Does this fix seem okay to you? |
::marker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions but I think that makes sense. Can't wait to remove the modifySelectors
code in v4 so that we only keep the new API and the container mutations directly 😄
src/corePlugins.js
Outdated
return | ||
} | ||
|
||
for (const varName of toRemove) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: use let
instead for consistency
function prepareBackup() { | ||
if (originals.size > 0) return // Already prepared, chicken out | ||
clone.walkRules((rule) => originals.set(rule, rule.selector)) | ||
// Already prepared, chicken out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests/variants.test.css
Outdated
.marker\:text-red-500 *::marker { | ||
color: rgb(239 68 68); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused about why this moved 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what's happening is the utilities are being grouped by utility now instead of by the selector, it's like:
.marker\:poop *::marker {}
.marker\:poop::marker {}
.marker\:banana *::marker {}
.marker\:banana::marker {}
.marker\:tonsils *::marker {}
.marker\:tonsils::marker {}
...instead of:
.marker\:poop *::marker {}
.marker\:banana *::marker {}
.marker\:tonsils *::marker {}
.marker\:poop::marker {}
.marker\:banana::marker {}
.marker\:tonsils::marker {}
I'd have to think about it harder but it feels like there was an important reason we originally did it by selector, so we probably want to be careful here. It's tricky admittedly for the reasons we talked about the other week, where variants registered within a function share a sort order or whatever.
I wonder if what we really want is to make this API possible for this one:
addVariant('marker', [() => {...}, () => {...}])
...so we can know the count up front and adjust the sort appropriately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good news @adamwathan that API already works :D — I've updated the PR to switch to it
9b24045
to
a761f7c
Compare
a761f7c
to
4e856b1
Compare
@RobinMalfait @adamwathan I've addressed your feedback — ping me if there's anything else that needs to be looked at. I'd like to merge this in and tag a release today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now!
The rules that use the
::marker
pseudo class only allow certain variables to be defined. In Safari it's likely there's an allowlist of some kind and it doesn't include custom properties. This prevents the default Tailwind text colors from working because we reference alpha values as the textOpcity plugin is on by default. To work around this you can disable the textOpacity plugin. This is definitely a browser bug that needs to be reported however it's also super not ideal that the out-of-the-box experience is broken for Tailwind here so we'll do something similar to our solution with::visited
and strip the text opacity property from rules and declarations.This additionally fixes a bug where mutations to
container
and use ofmodifySelector
did not preserve selectors when returning an array from a variant function.Basically it turns this:
into this:
Fixes #8610