-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[test] Generalize test utils #41175
[test] Generalize test utils #41175
Conversation
Netlify deploy previewhttps://deploy-preview-41175--material-ui.netlify.app/ Bundle size report |
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 👍🏼
describeConformance as baseDescribeConformance, | ||
ConformanceOptions, | ||
} from '@mui-internal/test-utils'; | ||
import { ThemeProvider, createTheme } from '@mui/material/styles'; |
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.
For the record, just in case another reviewer wonders:
For most components in the material-next
package, we're overriding these with CssVarsProvider
and extendTheme
from the material-next
package. For example: https://github.com/mui/material-ui/blob/master/packages/mui-material-next/src/ButtonBase/ButtonBase.test.tsx#L58
Using CssVarsProvider
and extendTheme
as default would break the tests on the material-next
package before we started the Material Design 3 implementations. So this is ok.
After v7, we must clean (remove?) the material-next
package.
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 for testing purposes (not just material-next), ThemeProvider
makes more sense than CssVarsProvider
because we don't need CSS vars generation in most tests.
Note: CssVarsProvider
uses ThemeProvider
internally
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.
Even though most material-next
components access theme.vars
exclusively? For example: https://github.com/mui/material-ui/blob/HEAD/packages/mui-material-next/src/Chip/Chip.tsx#L65
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.
👍
Removed @mui/material, @mui/base, and @mui/utils dependencies from @mui-internal/test-utils to break dependency cycles and allow the package to be published to npm.
Created wrappers in individual projects to maintain the old behavior.
This requires changes in MUI X. Integration PR: mui/mui-x#12130