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

Make --theme(…) return CSS variables #17036

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Mar 7, 2025

Closes #16945

This PR changes the --theme(…) function now return CSS var(…) definitions unless used in places where var(…) is not valid CSS (e.g. in @media (width >= theme(--breakpoint-md))):

/* input */
@theme {
  --color-red: red;
}
.red {
  color: --theme(--color-red);
}

/* output */
:root, :host {
  --color-red: red;
}
.red {
  color: var(--color-red);
}

Furthermore, this adds an --theme(… inline) option to the --theme(…) function to force the resolution to be inline, e.g.:

/* input */
@theme {
  --color-red: red;
}
.red {
  color: --theme(--color-red inline);
}

/* output */
.red {
  color: red;
}

This PR also changes preflight and the default theme to use this new --theme(…) function to ensure variables are prefixed correctly.

Test plan

  • Added unit tests and a test that pulls in the whole preflight under a prefix theme.

@philipp-spiess philipp-spiess requested a review from a team as a code owner March 7, 2025 12:26
@@ -51,15 +54,50 @@ function spacing(designSystem: DesignSystem, value: string, ...rest: string[]) {
return `calc(${multiplier} * ${value})`
}

function theme(designSystem: DesignSystem, path: string, ...fallback: string[]) {
function theme(designSystem: DesignSystem, source: AstNode, path: string, ...fallback: string[]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We're passing through the source now where we found the value that contains the function invocation.

@@ -29,7 +29,7 @@ export type DesignSystem = {
compileAstNodes(candidate: Candidate): ReturnType<typeof compileAstNodes>

getVariantOrder(): Map<Variant, number>
resolveThemeValue(path: string): string | undefined
resolveThemeValue(path: string, forceInline?: boolean): string | undefined
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 have to make this optional because of IntelliSense unfortunately. Also decided to make a boolean here since don't leak the ThemeOption enum beyond internal APIs right now.

Comment on lines 13 to 17
--default-font-feature-settings: var(--font-sans--font-feature-settings);
--default-font-variation-settings: var(--font-sans--font-variation-settings);
--default-mono-font-family: var(--font-mono);
--default-mono-font-feature-settings: var(--font-mono--font-feature-settings);
--default-mono-font-variation-settings: var(--font-mono--font-variation-settings);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that the --default-font-feature-settings theme variable is defined as:

@theme {
  --default-font-feature-settings: --theme(--font-sans--font-feature-settings, initial);
}

The value is resolved to initial (and thus omitted) because we know that --font-sans--font-feature-settings is not defined in the theme. Note that this implies that values referenced via --theme(…) can not be defined at runtime only and NEED to be in the @theme block. I think this is a reasonable thing to do since we also error when it's not in the @theme block (unless a fallback is specified like it is here).

// Inject the fallback…
//
// - …as the value if the value returned is `initial`
// - …expect any `initial` fallbacks on `var(…)`, `theme(…)`, or `--theme(…)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to say except?

This entire comment is kinda hard to parse for me

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 reworked this part, thanks

node.nodes.push({
kind: 'word',
value: `, ${fallback}`,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine but don't we technically treat the , as a separate node when parsing? Should we do that here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we do, the fallback could also consist of other, nested, functions etc. It's technically more correct to parse the fallback as well but it felt super wasteful to parse it just so it's stringified in the next line. If we ever change stuff so that the stringification won't work anymore if the word node contains stuff it shouldn't, we also have unit tests that would let us know this breaks so I decided to just put it in as a word node.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

`)
})

test('--theme(…) does not inject the fallback if the fallback is `initial`', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test name feels wrong. The different thing about this test is that the theme key exists and therefore the fallback isn't used.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

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

left some comments about test names, comments, and a thing about the AST

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.

v4 preflight theme variables aren't prefixed
2 participants