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

Improve support for custom variants in group-*, peer-*, has-*, and not-* variants #14743

Merged
merged 24 commits into from
Oct 24, 2024

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Oct 21, 2024

This PR does a number of of things that improve the ergonomics of using variants with group, peer, has, and not.

The body-less @variant syntax now creates at most one style rule

For example, in the following CSS:

@variant test (&:hover, &:focus);

The utility test:flex used to generate two separate rules:

.test\:flex {
  &:hover {
    display: flex;
  }
  &:focus {
    display: flex;
  }
}

But it now generates only one rule:

.test\:flex {
  &:hover, &:focus {
    display: flex;
  }
}

This makes the generated CSS more concise and is simpler for compound variants to work with.

group-*, peer-*, and has-* are clearer about what they support

One or more style rules MUST be present in the variant

A variant that is just a media query will not work with these variants:

@variant does-not-work {
  @media (min-width: 640px) {
    @slot;
  }
}

The utilities group-does-not-work:flex, peer-does-not-work:flex, and has-does-not-work:flex generate no CSS.

However, if a style rule is present, it will work:

@variant works {
  @media (min-width: 640px) {
    &:hover {
      @slot;
    }
  }
}

The utilities group-works:flex, peer-works:flex, and has-works:flex generate the following CSS:

.group-works\:flex {
  @media (min-width: 640px) {
    &:is(:where(.group):hover *) {
      display: flex;
    }
  }
}

.peer-works\:flex {
  @media (min-width: 640px) {
    &:is(:where(.peer):hover ~ *) {
      display: flex;
    }
  }
}

.has-works\:flex {
  @media (min-width: 640px) {
    &:has(*:hover) {
      display: flex;
    }
  }
}

Multiple style rules are okay

With nesting you can write a single rule like the following:

@variant test {
  &:hover, &[data-hover] {
    @slot;
  }
}

However you can also split this into two separate rules. It is functionally equivalent and works with group-*, peer-*, and has-*:

@variant test {
  &:hover {
    @slot;
  }

  &[data-hover] {
    @slot;
  }
}

Which generates the following CSS for group-test:flex, peer-test:flex, and has-test:flex:

.group-test\\:flex {
  &:is(:where(.group):hover *) {
    display: flex;
  }
  &:is(:where(.group)[data-hover] *) {
    display: flex;
  }
}

.peer-test\\:flex {
  &:is(:where(.peer):hover ~ *) {
    display: flex;
  }
  &:is(:where(.peer)[data-hover] ~ *) {
    display: flex;
  }
}

.has-test\\:flex {
  &:has(*:hover) {
    display: flex;
  }
  &:has(*[data-hover]) {
    display: flex;
  }
}

Nested style rules are NOT supported

We've so far ensured that nesting does not need to be flattened anywhere in core. Because of the nature of how group-*, peer-*, has-*, and not-* all work we'd have to implement a mechanism to flatten CSS ourselves. In the interest of keeping the core simpler we've decided to not support nested style rules in these variants.

So for example, the following variant will not work with group-*, peer-*, has-*, or not-*:

@variant test {
  &:hover {
    &:focus {
      @slot;
    }
  }
}

But if you manually flatten the nested style rules it will work:

@variant test {
  &:hover:focus {
    @slot;
  }
}

Don't worry nested at-rules still work — for example:

@variant this-works-too {
  @media (min-width: 640px) {
    @media (orientation: landscape) {
      &:hover:focus {
        @slot;
      }
    }
  }
}

produces the following CSS for group-this-works-too:flex, peer-this-works-too:flex, and has-this-works-too:flex:

.group-this-works-too\\:flex {
  @media (min-width: 640px) {
    @media (orientation: landscape) {
      &:is(:where(.group):hover:focus *) {
        display: flex;
      }
    }
  }
}

.peer-this-works-too\\:flex {
  @media (min-width: 640px) {
    @media (orientation: landscape) {
      &:is(:where(.peer):hover:focus ~ *) {
        display: flex;
      }
    }
  }
}

.has-this-works-too\\:flex {
  @media (min-width: 640px) {
    @media (orientation: landscape) {
      &:has(*:hover:focus) {
        display: flex;
      }
    }
  }
}

Expands on variants supported by not-*

The rules for not-* are very similar to the rules listed above but with some slight tweaks:

  • At-rules are supported as long as they are one of the conditional at-rules: @media, @supports, or @container
  • An @media at-rule must have one condition so a variant like @media screen, print will not work with not-*
  • At most one at-rule must be present in the variant
  • At most one style rule must be present in the variant
  • If an at-rule and style rule are present, the style rule must be nested within the at-rule or vice versa. They cannot be siblings.

The rules here are a bit complicated but hopefully you won't actually need to memorize these as most variants should end up being fairly simple in practice. We made sure to support these so things like not-hover:flex work as expected. These restrictions are in place to keep the core simple as implementing a full solution that handles arbitrarily nested at-rules and style-rules requires flattening nesting, interpreting CSS trees as logical expressions, and operating on those expressions. This is a non-trivial task that adds significant complexity to the codebase.

Body-less variants no longer produce logically incorrect CSS

The not-* variant as its currently implemented can produce logically incorrect CSS depending on how the variant was written. For example, the following variant:

@variant hocus (&:hover, &:focus);

Would generate the following CSS:

.hocus\:flex {
  &:not(:hover) {
    display: flex;
  }

  &:not(:focus) {
    display: flex;
  }
}

But with the first change we mentioned at the start of this description about body-less variants, this now generates the correct CSS:

.hocus\:flex {
  &:not(*:hover, *:focus) {
    display: flex;
  }
}

not-* now supports at-rule variants!

Now things like not-md:flex, not-supports-grid:flex, not-landscape, not-motion-safe, etc… all work as expected!

This includes custom variants too! For example, given the following CSS:

@variant hdr {
  @media (dynamic-range: high) {
    @slot;
  }
}

The utilities hdr:bg-red-900 and not-hdr:bg-red-900 generate the following CSS:

.not-hdr\\:bg-red-900 {
  @media not (dynamic-range: high) {
    background-color: var(--color-red-900, oklch(0.396 0.141 25.723));
  }
}

.hdr\\:bg-red-900 {
  @media (dynamic-range: high) {
    background-color: var(--color-red-900, oklch(0.396 0.141 25.723));
  }
}

This works with @supports and @container as well. For example, given the following CSS:

@variant grid {
  @supports (display: grid) {
    @slot;
  }
}

@variant contained-sm {
  @container (max-width: 640px) {
    @slot;
  }
}

The utilities grid:flex, not-grid:flex, contained-sm:flex, and not-contained-sm:flex generate the following CSS:

.not-grid\\:flex {
  @supports not (display: grid) {
    display: flex;
  }
}

.not-contained-sm\\:flex {
  @container not (max-width: 640px) {
    display: flex;
  }
}

.grid\\:flex {
  @supports (display: grid) {
    display: flex;
  }
}

.contained-sm\\:flex {
  @container (max-width: 640px) {
    display: flex;
  }
}

If your conditional at-rule already has a not we'll remove it — this works with @media, @supports, and @container.

For example, given the following CSS:

@variant flex-only {
  @supports not (display: grid) {
    @slot;
  }
}

The utility not-flex-only:flex generates the following CSS:

.not-flex-only\\:flex {
  @supports (display: grid) {
    display: flex;
  }
}

@thecrypticace thecrypticace marked this pull request as ready for review October 21, 2024 21:34
@thecrypticace thecrypticace requested a review from a team as a code owner October 21, 2024 21:34
Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit confusing but the results are really nice! I left some inline comments, mostly regarding the new compounds property that we need to retain in the Variants map, but once this is more clear, let's just get this merged and hopefully not think about it again lol

packages/tailwindcss/src/variants.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/variants.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@thecrypticace
Copy link
Contributor Author

thecrypticace commented Oct 22, 2024

TODOs:

  • arbitrary variants need to be looked at to determine what type they are (same mechanism that we use for staticVariant)
  • Inspect @variant to classify the variant also in the same manner as staticVariant
  • Also for the statically analyzable versions of addVariant

@thecrypticace thecrypticace marked this pull request as draft October 22, 2024 21:18
@thecrypticace thecrypticace marked this pull request as ready for review October 23, 2024 11:22
@tailwindlabs tailwindlabs deleted a comment from thecrypticace Oct 23, 2024
Copy link
Member

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few things I noticed

CHANGELOG.md Outdated Show resolved Hide resolved
packages/tailwindcss/src/index.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/intellisense.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/intellisense.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/variants.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/variants.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/candidate.test.ts Outdated Show resolved Hide resolved
@thecrypticace thecrypticace merged commit 148d870 into next Oct 24, 2024
1 check passed
@thecrypticace thecrypticace deleted the feat/v4-not-tweaks branch October 24, 2024 17:27
Copy link
Member

@adamwathan adamwathan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of code here so might have missed it but I don't see tests for not-* with arbitrary values other than not-[:checked] — we should probably add tests for more complex arbitrary scenarios too.

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- Added `not-*` versions of all builtin media query and supports variants ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Added `not-*` versions of all builtin media query and supports variants ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))
- Support `not-*` with all built-in media query and `supports-*` variants ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))

### Added

- Added `not-*` versions of all builtin media query and supports variants ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))
- Improved support for custom variants with `group-*`, `peer-*`, `has-*`, and `not-*` ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to be clearer/more specific about what's actually possible now that wasn't before? I try to be really careful about using the format "Improve …" in changelog entries because it's not really clear what has actually changed

@@ -17,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Don't migrate important modifiers inside conditional statements in Vue and Alpine (e.g. `<div v-if="!border" />`) ([#14774](https://github.com/tailwindlabs/tailwindcss/pull/14774))
- Ensure third-party plugins with `exports` in their `package.json` are resolved correctly ([#14775](https://github.com/tailwindlabs/tailwindcss/pull/14775))
- Ensure underscores in the `url()` function are never unescaped ([#14776](https://github.com/tailwindlabs/tailwindcss/pull/14776))
- Fixed display of complex variants in Intellisense ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is meaningfully more specific but at least should use imperative mood for changelog entries to be consistent with other ones:

Suggested change
- Fixed display of complex variants in Intellisense ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))
- Ensure complex variants are displayed correctly in IntelliSense completions ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743))

Also capitalized "IntelliSense" consistent with how we do it elsewhere 👍

}

compound(
compoundWith(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all compound variants need to specify their "with" do we actually need to change this name? I liked how before these methods were named as sort of factory functions for the variant kind, like static, functional, and compound. I don't think we want to think of these as "compoundWith" variants do we, vs. still just "compound"?

}

// Pseudo-elements are present so we can't compound
if (sel.includes('::')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to think about ::file-selector-button as special here? I have tested a few things and don't think so, because in my testing :not(::file-selector-button) doesn't work in any browser even though stuff like ::file-selector-button:is(:not(:active)) does work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we don't — pseudo-elements don't work in :not(…), :is(…), :where(…), :has(…) and can't appear in the middle of a selector so .group and .peer are out too.

As for your 2nd example I think that's file:not-active:{utility} yeah?

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.

4 participants