Support of swapping underlying variables while using hierarchical variable references about stylex HOT 13 OPEN

Thanks @necolas, and @aspizu for pointing out what I've missed.

I wrote a quick patch to StyleX and can now make it achieve what I expected (output of the patched version of StyleX: https://codesandbox.io/p/sandbox/stylex-token-hierarchy-esbuild-example-output-240601-vdwvtg).

Maybe such behavior can be made a config to enable? Or, due to the principle to avoid global configuration for StyleX, let stylex.createTheme accept an option argument for it to also add the base theme class name?

from stylex.

Is there a workaround for this at the moment? My application has 5 different color themes, and I would like to create some variables that reference the theme colors so I don't have to access the theme colors directly. For example, I want all focused elements to have the outline color primary400. I don't want to use this variable directly because it is error prone (someone could use 300 or 500 by mistake), so I would like to use a different variable outlineColor that abstracts this away.

// themes.stylex.ts
export const redBlueTheme = stylex.createTheme(themeColors, { ... });
export const purpleYellowTheme = stylex.createTheme(themeColors, { ... });
export const greenPinkTheme = stylex.createTheme(themeColors, { ... });

// variables.ts
export const themeColors = stylex.defineVars({
  primary100: '...',
  primary200: '...',
  // ...
});

export const systemColors = stylex.defineVars({
  outlineColor: themeColors.primary400, // <- This does not use the current theme
});

from stylex.

React Strict DOM works exactly as you're expecting, on native - https://snack.expo.dev/gviZCT1uY_KjFgWow6u7m?platform=android. We need StyleX to add a proper version of this simple patch to get web to match.

As applying the x className in your example wipes out all other themes and resets everything back to default.

This is intentional.

from stylex.

This is probably a bug given how you'd expect things to work.

I think we can get the expected result by including the base theme's class alongside the sub-theme class. StyleX could do this automatically when returning createTheme styles.

https://codesandbox.io/p/sandbox/stylex-vars-65yyjg?file=%2Findex.html%3A10%2C9

There has been previous talk about needing to expose the base theme as a style anyway.

from stylex.

The problem comes from CSS itself (#566). There might be issues with re-declaring custom properties which inherit other custom properties, What if there is a theme which changes that particular custom property to have a constant fixed value? The compiler would have to figure out how to handle dynamic values set by javascript code.

from stylex.

@aspizu your example and expected result is also possible: https://codesandbox.io/p/sandbox/stylex-vars-65yyjg?file=%2Findex.html%3A5%2C13. What are the cases where this approach wouldn't work?

from stylex.

Nice. I think this is how StyleX theming should work by default, but let's see what Naman thinks

from stylex.

I like this!

from stylex.

I've given this thought before and it's not as simple. The idea here is that variables should work like reactive values. This sometimes feels like the way things should work but this is not always the case.

I think safest way to make this work is by using defineConsts instead of defineVars for certain layers. This would obviously mean that the constant cannot be redefined. But for constants we could basically erase them at compile time so they are "reactive".

Would that be a good solution?

from stylex.

What's wrong with the approach we discussed above? And what is the output code that makes values "reactive"?

from stylex.

And what is the output code that makes values "reactive"?

My proposal is that we can do handle constants differently where they don't exist after compilation at all.

So taking the original example:

// tokens.stylex.ts

import * as stylex from '@stylexjs/stylex';

/** Reference tokens for color palette */
export const colorPalette = stylex.defineVars({
  blue: '#007AFF',
  indigo: '#5856D6',
  white: '#F2F2F7',
  // ...
});

/** System tokens for color */
export const systemColors = stylex.defineVars({
  primary: colorPalette.blue,
  onPrimary: colorPalette.white,
  // ...
});

export const buttonTokens = stylex.defineConsts({
  primaryColor: systemColors.primary,
  primaryLabelColor: systemColors.onPrimary,
  // ...
});

This will generate following CSS:

:root {
    --systemColors_primary: var(--colorPalette_blue);
    /* ... */          │
}                      ╰───────────────────────────────╮
/*                                                     │
:root {                                                ↓
    --buttonTokens_primaryColor: var(--systemColors_primary);
    ... */                  │
}                           ╰─────────────────────────────╮
*/                                                        │
.indigoTheme {                                            │
    --systemColors_primary: var(--colorPalette_indigo);   │
    /* ... */                                             │
}                                      ╭──────────────────╯
                                       │
.button {                              ↓
    color: var(--systemColors_primary); /* var(--buttonTokens_primaryColor) */
    /* ... */
}

So essentially, any "constant" is replaced by its value at compile time so the extra layers are a pure abstraction for DX only and does not exist at all at runtime.

IMO, this should solve the core problem described here. In fact, systemColors could probably be defined as constants as well for better efficiency.

I'm thinking of some examples to explain why making the system work as the OP suggested is problematic.

What's wrong with the approach we discussed above?

This codesandbox might show the problem with your approach.

There is an edge-case with applying any other theme. As applying the x className in your example wipes out all other themes and resets everything back to default.

from stylex.

As applying the x className in your example wipes out all other themes and resets everything back to default.

This is intentional.

I'm not convinced this is the right solution, but the global option in #631 should enable this behaviour. I think the group option makes the most sense.

It made sense to make this config option for now to avoid breaking changes, we should be able to experiment with the options and change the default in a future update soon.

from stylex.

Is there an update on this?

from stylex.