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

generate vars in extendTheme #36

Conversation

siriwatknp
Copy link

@siriwatknp siriwatknp commented Jan 13, 2023

Propose

If we want extendTheme() to produce theme.vars, the logic of producing variables should be in extendTheme(). I think it makes a lot of sense.

One major change is the shouldSkipGeneratingVar will be in extendTheme instead of CssVarsProvider. I think this should be fine since we haven't documented this configuration.

const theme = extendTheme({
+  shouldSkipGeneratingVar: (keys) => …,
})

<CssVarsProvider
- shouldSkipGeneratingVar={(keys) => …}
/>

Changes

Before:

After

Generating CSS variables outside of CssVarsProvider sounds like a better abstraction because it does not require the current colorScheme or mode of the application. It just prepare the variables for all of the color schemes.


Copy link
Owner

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The reason why I didn't go on this route, is because for Material You, I was aiming to make the components work without using CssVarsProvider, so my thought was that the theme.vars should point to the actual value, not a CSS var. Should we resolve this first? Make a decisions about this before merging the PR.

@siriwatknp
Copy link
Author

The reason why I didn't go on this route, is because for Material You, I was aiming to make the components work without using CssVarsProvider, so my thought was that the theme.vars should point to the actual value, not a CSS var

I am afraid that if theme.vars point to the actual value, would it be misleading?

I was aiming to make the components work without using CssVarsProvider

I am not sure it it related to extendTheme() returning theme.vars or not. Can you elaborate?

@siriwatknp
Copy link
Author

siriwatknp commented Feb 7, 2023

I am not sure it it related to extendTheme() returning theme.vars or not. Can you elaborate?

Ah, now I get it.

@mnajdova mnajdova merged commit cc2f375 into mnajdova:extendTheme/generate-vars Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants