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

Create variants for aria, supports, and data JS config theme keys #14407

Merged

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Sep 12, 2024

This PR adds support for the aria, supports, and data properties found in JS config options. In v3, you could extend the theme to add more variants by using an object syntax like this:

{
   theme: {
    extend: {
      aria: {
        polite: 'live="polite"',
      },
      supports: {
        'child-combinator': 'h2 > p',
      },
      data: {
        checked: 'ui~="checked"',
      },
    },
  }
}

Since we no longer rely on theme variables for these variants, the way to make this work is by adding custom variants for each of these manually added variants.

@philipp-spiess philipp-spiess changed the title Create variants for aria, supports, and data JS theme keys Create variants for aria, supports, and data JS config theme keys Sep 12, 2024
@adamwathan
Copy link
Member

Should we actually replace the root data/supports/aria variants here maybe so all the custom ones stay grouped with the arbitrary ones in the generated CSS?

@philipp-spiess philipp-spiess force-pushed the feat/aria-supports-data-variants-from-theme-config branch from 94e557a to ab305dd Compare September 12, 2024 11:52
@philipp-spiess
Copy link
Member Author

Should we actually replace the root data/supports/aria variants here maybe so all the custom ones stay grouped with the arbitrary ones in the generated CSS?

Can you think of ways where the grouping would be important here? I feel like it would be weird to replace core utilities that are perfectly functioning. Makes it feel brittle because we could create a wrong implementation at any of the two places now or we forget to update one, etc.

@philipp-spiess philipp-spiess marked this pull request as ready for review September 12, 2024 12:02
@philipp-spiess philipp-spiess force-pushed the feat/aria-supports-data-variants-from-theme-config branch from ab305dd to 41d2606 Compare September 12, 2024 13:22
@adamwathan
Copy link
Member

Can you think of ways where the grouping would be important here?

@philipp-spiess Here's an example of where it would be a breaking change:

v3 registering aria-current in the config

https://play.tailwindcss.com/noQ9ZX69LZ

v4 adding a new separate aria-current variant

https://play.tailwindcss.com/qFyxAsRDt6

Likelihood of affecting anyone is quite small realistically but it is technically a breaking change.

Another option is to explicitly register the custom ones with the same order as the corresponding existing variants. This isn't possible with our currently exposed APIs I don't think but I'm sure we could come up with a way to do it by just making it possible to pass an explicit order when registering a variant but making it optional.

@philipp-spiess
Copy link
Member Author

philipp-spiess commented Sep 12, 2024

@adamwathan Ahh, thank you for the examples, I see the issue now 🤕. Yea we'd have to make it so that we can overwrite the order and then do a design system lookup for the default variants. What I don't like about this is that now we have to add a new API to core that is only needed for the compatibility work. When we eventually remove it, we will for sure forget to remove this new order option too... Hmmm.

We could make it work I think by kinda like "patching" the default functional variants? It seems like overwriting a utility does not change it's order! Let me try something. 👀

Edit: I realize this is probably what you have meant by your initial comment all along 🤦

@philipp-spiess philipp-spiess force-pushed the feat/aria-supports-data-variants-from-theme-config branch from 41d2606 to 669fd15 Compare September 12, 2024 14:52
@philipp-spiess
Copy link
Member Author

@adamwathan Check out this tech: 5dcb301

@philipp-spiess philipp-spiess force-pushed the feat/aria-supports-data-variants-from-theme-config branch from 95f7cc6 to d3143b9 Compare September 17, 2024 11:27
packages/tailwindcss/src/compat/config.test.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/compat/config.test.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/compat/theme-variants.ts Outdated Show resolved Hide resolved
packages/tailwindcss/src/compat/theme-variants.ts Outdated Show resolved Hide resolved
@adamwathan
Copy link
Member

Had a really old review I started and forgot to submit so just submit it now before I forget, probably had some outdated stuff and haven’t looked at everything yet either, on phone heh.

@philipp-spiess philipp-spiess merged commit 9ef4aaa into next Sep 17, 2024
3 checks passed
@philipp-spiess philipp-spiess deleted the feat/aria-supports-data-variants-from-theme-config branch September 17, 2024 15:09
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.

3 participants