-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
[docs][material-next] Add contributing guide #39944
[docs][material-next] Add contributing guide #39944
Conversation
Netlify deploy previewhttps://deploy-preview-39944--material-ui.netlify.app/ Bundle size report |
@@ -13,3 +13,10 @@ Material UI v6's notable changes compared to v5 are: | |||
- Implement [Material You](https://m3.material.io/) (Material Design 3). | |||
|
|||
For migration steps when upgrading from v5, follow the [migration guide](/packages/mui-material-next/migration.md). | |||
|
|||
## Contributing |
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.
Copy-pasta from mui-base/README.md 😬
4. Drop support for `ThemeProvider` in favor of `CssVarsProvider`. In practice, this means: | ||
- Consuming tokens from `theme.vars` instead of `theme` | ||
- In tests, using `CssVarsProvider` and `extendTheme` (both imported from `@mui/material-next/styles`) instead of `ThemeProvider` and `createTheme`, as well as providing the same `CssVarsProvier` and `extendTheme` to `describeConformance`'s `ThemeProvider` and `createTheme` options. | ||
5. Refactor component to use Base UI hooks if it exists |
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.
5. Refactor component to use Base UI hooks if it exists |
We can drop this, it can be added later, especially if we aim at releasing only Material You as a change in v6 without other unexpected breaking changes
- Try to avoid breaking changes, keeping the component’s API the same: | ||
- An exception to this is to use Material You nomenclature and naming conventions, even if it would be a breaking change. | ||
- If breaking changes are inevitable, then document them right away in `/packages/mui-material-next/migration.md` and add the `breaking change` label to your PR. | ||
- Divide the work in whatever way makes more sense. One PR for all steps, one PR for each step, or anything in between |
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.
- Divide the work in whatever way makes more sense. One PR for all steps, one PR for each step, or anything in between | |
- Divide the work in whatever way makes more sense. One PR for few steps or one PR for each step, however keep in mind that smaller pull request will be reviewed faster |
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.
Nice! I agree with @mnajdova's comments, after fixing those this is RTM.
Updated and added some more tweaks ~ |
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.
LGTM 🎉
When it's merged, we should remove it from the issue and add a link to this one instead 🙌🏼
I copied the contributing guide from #29345 to make it a bit more visible, and to un-bloat that issue a bit ~
We can replace the content in the issue with a link once this is merged