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

Enforce the order of some variants #6018

Merged
merged 3 commits into from Nov 12, 2021
Merged

Conversation

RobinMalfait
Copy link
Contributor

@RobinMalfait RobinMalfait commented Nov 8, 2021

The order of variants can be important for certain scenario's.

For example, if you have:

<div class="group-hover:dark:text-center"></div>

This would result in:

.group:hover .dark .group-hover\:dark\:text-center {
  text-align: center;
}

This requires the following DOM structure:

<div class="group">
  <div class="dark">
    <div class="group-hover:dark:text-center"></div>
  </div>
</div>

However, switching the order might be useful.

<div class="dark:group-hover:text-center"></div>

This would result in:

.dark .group:hover .dark\:group-hover\:text-center {
  text-align: center;
}

This requires the following DOM structure:

<div class="dark">
  <div class="group">
    <div class="group-hover:dark:text-center"></div>
  </div>
</div>

This is a contrived example, but there are scenarios (see our Typography plugin) where it does make sense to switch the order sometimes.
However, in some scenario's it doesn't make sense because you would generate invalid css selectors (well.. not invalid, but selectors that won't work).

Such an example is when you have hover:before:text-center vs before:hover:text-center. This would result in:

.hover\:before\:text-center::before:hover { /* This is useless, because you can't hover a ::before element */
  content: var(--tw-content);
  text-align: center;
}

.before\:hover\:text-center:hover::before {
  content: var(--tw-content);
  text-align: center;
}

To fix this, we move all the pseudo elements to the back of the selector, otherwise the selector wouldn't even work. This is implemented on a selector level and not on a variant API level. Which means that this would also work for third party plugins.
With this PR, the examples we used earlier (hover:before:text-center vs before:hover:text-center) would result in:

.hover\:before\:text-center:hover::before { /* This now has :hover::before, even if the class in the HTML is different */
  content: var(--tw-content);
  text-align: center;
}

.before\:hover\:text-center:hover::before {
  content: var(--tw-content);
  text-align: center;
}

The cool part here is that we didn't change the base selector found in the HTML. This is because of the new addVariant API introduced in #5809.
We only append and prepend selector parts to the existing base class (the candidate found in the HTML), instead of re-building the selector from scratch.

This PR does mean that if you used hover:before:text-center in one file, and before:hover:text-center in the other, that there is a bit of duplication going on. Maybe that's fine, maybe in the future we can warn about this to enforce consistency. But at least it will work now instead of introducing a bug.

Fixes: #6016

src/lib/generateRules.js Outdated Show resolved Hide resolved
src/lib/generateRules.js Outdated Show resolved Hide resolved
@RobinMalfait RobinMalfait force-pushed the enforce-before-and-after-order branch 4 times, most recently from d6f31fb to 1bfd575 Compare November 12, 2021 15:16
@RobinMalfait RobinMalfait merged commit a3579bc into master Nov 12, 2021
@RobinMalfait RobinMalfait deleted the enforce-before-and-after-order branch November 12, 2021 15:38
david-crespo added a commit to oxidecomputer/console that referenced this pull request Nov 17, 2021
* upgrade to tailwind 3.0 alpha, make config changes

* rewrite svg: w/ new addVariant API tailwindlabs/tailwindcss#5809

* first first-of-type:before: order issue (tw fix in next alpha)

tailwindlabs/tailwindcss#6112
tailwindlabs/tailwindcss#6016
tailwindlabs/tailwindcss#6018

* replace tailwindcss-children plugin with new custom addVariant

* oh yeah... use stroke-green-500

* each variant doesn't need its own plugin

* update yarn.lock after merging main
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.

In v3 alpha, variant combinations like hover:before: aren't working
1 participant