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

[Joy] Remove private to leverage CodeSandbox #29280

Merged
merged 9 commits into from
Oct 28, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Oct 25, 2021

Summary

  • mark package as public starting with v0.1.0-alpha.0 to make joy appear in CodeSandbox.
    • It is fine to release 0.x.y because it is considered unstable according to semver
  • add joy creact-react-app to examples so that we can try it out in CodeSandbox.
  • add a dummy Button.

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 25, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 069dcd0

@siriwatknp siriwatknp changed the title [Joy] Publish to npm to leverage CodeSandbox [Joy] Remove private to leverage CodeSandbox Oct 25, 2021
@siriwatknp siriwatknp mentioned this pull request Oct 25, 2021
1 task
@michaldudak
Copy link
Member

@mui/material-next uses 6.0.0-alpha. Have you considered adopting the same pattern here as well?

@mnajdova
Copy link
Member

@mui/material-next uses 6.0.0-alpha. Have you considered adopting the same pattern here as well?

👍 replace dev with alpha please :)


export default function App() {
return (
<CssVarsProvider>
Copy link
Member

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 :)

Copy link
Member

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?

Copy link
Member Author

@siriwatknp siriwatknp Oct 26, 2021

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.

Copy link
Member

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.

Copy link
Member Author

@siriwatknp siriwatknp Oct 27, 2021

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.

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 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.

@siriwatknp
Copy link
Member Author

@mui/material-next uses 6.0.0-alpha. Have you considered adopting the same pattern here as well?

At first I thought that using dev would be more explicit that this is development work but I am happy to go with alpha.

@siriwatknp siriwatknp requested a review from mnajdova October 27, 2021 02:37
Copy link
Member

@hbjORbj hbjORbj left a 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 :)

@siriwatknp
Copy link
Member Author

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).

@siriwatknp siriwatknp merged commit a1a2aef into mui:master Oct 28, 2021
@zannager zannager added the design: joy This is about Joy Design, please involve a visual or UX designer in the process label Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: joy This is about Joy Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants