-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[Joy] Remove private
to leverage CodeSandbox
#29280
Conversation
private
to leverage CodeSandbox
@mui/material-next uses 6.0.0-alpha. Have you considered adopting the same pattern here as well? |
👍 replace |
|
||
export default function App() { | ||
return ( | ||
<CssVarsProvider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would suggest removing it, to indicate that it is not necessary if no theme is provided, or provide a custom theme :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, probably the theme should be required, no? Otherwise, what is the point of adding a provider without the theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think CssVarsProvider
is what we will use the most, so I put it as default here. (We can change it in the future if we have a better way but for now I think it is good enough)
probably the theme should be required
The theme is not required because of the default theme. However, CssVarsProvider
is required, otherwise, how do we turn the default theme to CSS variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I thought joy's default theme already crates the variables. Does this mean that whoever uses the joy's design system will need to wrap the app in CssVarsProvider
? If yes, then this breaks the zero configuration approach we had till now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that whoever uses the joy's design system will need to wrap the app in CssVarsProvider
No. You can start with plain Button
same as @mui/material
:
import Button from '@mui/joy/Button';
<Button> // this should work
You are still able to access the default theme as usual but you won't get the global theme CSS variables attached to your app (some components might have CSS variables because it is the component scope).
If you want to use CSS variables from your theme, you will need to wrap your app with CssVarsProvider
.
You can consider CssVarsProvider
as a feature on top of ThemeProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still reluctant about attaching CSS variables automatically without using CssVarsProvider
. I think it can be tested in the future but not a blocker at this point.
At first I thought that using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me :)
I think it is fine to merge this PR to unblock some of my WIP (All of the change is about Joy, so I don't think there will be any impact). |
Summary
v0.1.0-alpha.0
to make joy appear in CodeSandbox.