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

Fix var(…) as the opacity value inside the theme(…) function #14653

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

RobinMalfait
Copy link
Member

Inside the theme(…) function, we can use the / character for applying an opacity. For example theme(colors.red.500 / 50%) will apply a 50% opacity to the colors.red.500 value.

However, if you used a variable instead of the hardcoded 50% value, then this was not parsed correctly. E.g.: theme(colors.red.500 / var(--opacity))

If we have this exact syntax (with the spaces), then it parses, but some information is lost:

<div class="bg-[theme(colors.red.500_/_var(--opacity))]"></div>

Results in:

.bg-\[theme\(colors\.red\.500_\/_var\(--opacity\)\)\] {
  background-color: color-mix(in srgb, #ef4444 calc(var * 100%), transparent);
}

Notice that the var(--opacity) is just parsed as var, and the --opacity is lost.

Additionally, if we drop the spaces, then it doesn't parse at all:

<div class="bg-[theme(colors.red.500/var(--opacity))]"></div>

Results in:

This means that we have to handle 2 issues to make this work:

  1. We have to properly handle the / character as a proper separator.
  2. If we have sub-functions, we have to make sure to print them in full (instead of only the very first node (var in this case)).

@RobinMalfait RobinMalfait changed the title Fix theme(.../var(--foo)) syntax Fix var(…) as the opacity value inside the theme(…) function Oct 11, 2024
@@ -136,6 +136,27 @@ describe('theme function', () => {
`)
})

test('theme(colors.red.500/75%)', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same test as below, but without spaces. Already worked, but added for completeness sake.

}
.red {
/* prettier-ignore */
color: theme(colors.red.500/var(--opacity,50%));
Copy link
Member Author

Choose a reason for hiding this comment

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

Prettier injected a space var(--opacity, 50%), but explicitly didn't want it so I added the prettier-ignore comment.

CHANGELOG.md Outdated
@@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Pass options when using `addComponents` and `matchComponents` ([#14590](https://github.com/tailwindlabs/tailwindcss/pull/14590))
- Ensure `boxShadow` and `animation` theme keys in JS config files are accessible under `--shadow-*` and `--animate-*` using the `theme()` function ([#14642](https://github.com/tailwindlabs/tailwindcss/pull/14642))
- Ensure all theme keys with new names are also accessible under their old names when using the `theme()` function with the legacy dot notation syntax ([#14642](https://github.com/tailwindlabs/tailwindcss/pull/14642))
- Fix `var(…)` as the opacity value inside the `theme(…)` function ([#14653](https://github.com/tailwindlabs/tailwindcss/pull/14653))
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
- Fix `var(…)` as the opacity value inside the `theme()` function ([#14653](https://github.com/tailwindlabs/tailwindcss/pull/14653))
- Allow `var(…)` for the opacity modifier inside `theme([path] / [modifier])` function calls ([#14653](https://github.com/tailwindlabs/tailwindcss/pull/14653))

Maybe if we word it like this it's also an addition and not a fix? I’m not sure about the theme function arguments, maybe it's fine to abbreviate with but I was wondering if this makes it clearer what the modifier position is in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be a fix because without it we would generate invalid CSS

Copy link
Member Author

@RobinMalfait RobinMalfait Oct 14, 2024

Choose a reason for hiding this comment

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

How about something like this?

Ensure `var(…)` can be used as the opacity value inside the `theme([path] / [modifier])` function

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

@RobinMalfait RobinMalfait force-pushed the fix/var-in-theme-function branch from 54c3987 to 2ef8891 Compare October 14, 2024 11:48
@RobinMalfait RobinMalfait enabled auto-merge (squash) October 14, 2024 12:04
This means that we have to handle 2 cases:
1. Handle the `/` as a separator
2. Stringify the AST node, instead of using the value, because the
   `var(--opacity)` part is a (nested) AST node.
@RobinMalfait RobinMalfait force-pushed the fix/var-in-theme-function branch from 5926c05 to b0ed890 Compare October 14, 2024 12:04
@RobinMalfait RobinMalfait merged commit f2ebb8e into next Oct 14, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the fix/var-in-theme-function branch October 14, 2024 12:08
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