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

Remove automatic var(…) injection #13657

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented May 8, 2024

This PR is a continuation of #13537.

Currently we reverted the merged changes so that we can get a new alpha version out without this change.


This PR removes automatic var(…) injection for arbitrary properties, values and modifiers.

There are a few properties that use "dashed-ident" values, this means
that you can use --my-thing as-is without the var(…) around it.

E.g.:

.foo {
  /* Notice that these don't have `var(…)` */
  view-timeline-name: --timeline-name; 
  anchor-name: --sidebar;
}

This causes issues because we are now injecting a var(…) where it's not needed.

One potential solution is to create a list of properties where dashed idents can be used. However, they can also use CSS custom properties that point to another dashed ident.

E.g.:

.foo {
  --target: --sidebar;
  anchor-name: var(--target);
}

A workaround that some people used is adding a _ in front of the variable: mt-[_--my-thing] this way we don't automatically inject the var(…) around it. This is a workaround and gross.

While the idea of automatic var injection is neat, this causes more trouble than it's worth. Adding var(…) explicitly is better.

A side effect of this is that we can simplify the internals for the candidate data structure because we don't need to track dashedIdent separately anymore.

@RobinMalfait RobinMalfait added the v4 label May 8, 2024
Copy link

@Pyr33x Pyr33x left a comment

Choose a reason for hiding this comment

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

LGTM!

@RobinMalfait RobinMalfait force-pushed the feat/remove-var-shorthand branch 3 times, most recently from 55533cc to e6c54ea Compare September 23, 2024 09:38
Comment on lines -769 to -774
'bg-red-[--value]',
'bg-red[--value]',

'bg-red-[--value]!',
'bg-red[--value]!',

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped because wrapping them in var(…) results in the same classes as below

Comment on lines +1579 to 1580
'supports-[var(--test)]:flex',
'supports-[--test]:flex',
Copy link
Member Author

Choose a reason for hiding this comment

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

Added an additional test case since this results in 2 different checks.

This also gets rid of the `dashedIdent` property because the arbitrary
value now contains it as-is.
@RobinMalfait RobinMalfait merged commit a270e2c into next Sep 23, 2024
3 checks passed
@RobinMalfait RobinMalfait deleted the feat/remove-var-shorthand branch September 23, 2024 19:57
philipp-spiess added a commit that referenced this pull request Oct 1, 2024
In v4, we're [removing automatic var
injection](#13657)
(please refer to this PR for more detail as to why).

Automatic var injection made it so that if you have a candidate like
`bg-[--my-color]`, v3 would automatically wrap the content of the
arbitrary section with a `var(…)`, resulting in the same as typing
`bg-[var(--my-color)]`.

This PR adds codemods that go over various arbitrary fields and does the
`var(…)` injection for you. To be precise, we will add `var(…)` to:

- Modifiers, e.g.: `bg-red-500/[var(--my-opacity)]`
- Variants, e.g.:  `supports-[var(--test)]:flex`
- Arbitrary candidates, e.g.: `[color:var(--my-color)]`
- Arbitrary values for functional candidates, e.g.:
`bg-[var(--my-color)]`

---------

Co-authored-by: Robin Malfait <malfait.robin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants