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

Support built-in variants for utilities that include pseudo-elements #970

Merged
merged 3 commits into from Jul 6, 2019

Conversation

adamwathan
Copy link
Member

This PR tries to improve the behavior of our built-in hover, active, focus, and focus-within variant plugins to handle utilities that include pseudo-elements more gracefully.

Prior to this PR, this code:

@variants focus {
  .placeholder-gray-400::placeholder { color: theme('colors.gray.400') }
}

...would generate this CSS:

.placeholder-gray-400::placeholder { color: #CBD5E0 }
.focus\:placeholder-gray-400:focus { color: #CBD5E0 }

Notice that the ::placeholder pseudo-element is completely gone in the focus variant.

This PR changes this behavior to create this output:

.placeholder-gray-400::placeholder { color: #CBD5E0 }
.focus\:placeholder-gray-400:focus::placeholder { color: #CBD5E0 }

This feels much more correct to me at least in this situation, but there is a concern that for some reason using some pseudo-element, someone might actually want this sort of output:

.hover\:foo::before:hover { ... }

...where the hover pseudo-class is added to the pseudo-element itself, rather than the element to which the pseudo-element belongs.

I'm not convinced it's worth trying to solve that problem right now as I don't believe it's possible to give the end user control over how this works without introducing a breaking change, but the new behavior in this PR certainly feels like an improvement over what is currently completely broken.

One thing to note is that the specifics of this implementation actually cause all classes in a selector to receive the pseudo-class prefix:

/* Input */
@variants focus {
  .foo .bar .baz { ... }
}

/* Output */
.hover\:foo:hover .hover\:bar:hover .hover\:baz:hover { ... }

I don't really want people to depend on this and consider it undefined behavior, but can't decide if it's worth trying to detect and throwing an explicit error, mostly because I'm unsure exactly what the heuristic would be for "this is weird don't do this". Would it be any selector that includes multiple classes? An element + a class?

Also, anyone who has authored a variant plugin (like for disabled or something) may want to update their own implementations to behave the way this PR behaves, which makes me wonder if I should try to implement this in a way that makes that automatic, although I'm pretty convinced that couldn't be done in a non-hacky non-bandaid-y way.

Feedback from variant plugin authors on this PR overall would be appreciated.

@adamwathan
Copy link
Member Author

Reminder to myself: think about if this impacts group-hover and if that plugin needs to be updated to accommodate the same thing.

@adamwathan adamwathan merged commit da24fba into master Jul 6, 2019
@adamwathan adamwathan deleted the support-pseudo-elements-with-variants branch July 6, 2019 10:11
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

1 participant