-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
[pickers] Finish the migration to be compatible with PigmentCSS #13643
base: master
Are you sure you want to change the base?
[pickers] Finish the migration to be compatible with PigmentCSS #13643
Conversation
Deploy preview: https://deploy-preview-13643--material-ui-x.netlify.app/ |
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 effort, thanks! 👍
Leaving some clarification questions.
import { SingleInputTimeRangeFieldProps } from './SingleInputTimeRangeField.types'; | ||
import { useSingleInputTimeRangeField } from './useSingleInputTimeRangeField'; | ||
import { FieldType } from '../models'; | ||
|
||
const useThemeProps = createUseThemeProps('MuiSingleInputTimeRangeField'); |
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.
Nitpick: Do you know if it is mandatory to duplicate the name
prop?
Is it used in static extraction or something else? 🤔
Currently, it seems a bit strange. 🙈
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.
Honestly I have no idea
I think I read it month ago. I'd say it's just that PigmentCSS will replace it be an import links it to the build-time theme based on the name that is in createUseThemeProps
and ignore the one of useThemeProps
and we kept the one in useThemeProps
to avoid a breaking change.
@@ -1,16 +1,19 @@ | |||
import * as React from 'react'; | |||
import clsx from 'clsx'; | |||
import { styled, alpha, useThemeProps } from '@mui/material/styles'; | |||
import { alpha } 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.
Will keeping the alpha
call outside of zero-styled
allow for static extraction? 🤔
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 so, but @brijeshb42 can confirm/infirm this one 👍
@@ -2,13 +2,13 @@ import * as React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import clsx from 'clsx'; | |||
import { useRtl } from '@mui/system/RtlProvider'; | |||
import { styled, useThemeProps } 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.
To keep the static-extraction intact we should avoid adding new imports from @mui/material/styles
directly.
WDYT about creating an ESLint rule for that? 🤔
However, we'd have to think of what to do with type imports... Maybe import type
could be skipped by an ESLint rule? 🤔
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.
@brijeshb42 is this something that is planned on the core side?
I guess it could benefit all MUI teams trying to migrate and to avoid re-using direct imports by mistake.
I don't think we can totally bloke @mui/material/styles
since we want to use useTheme
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.
Makes sense. We'd need too many ignores for those. 🙈
I don't think we can totally bloke
@mui/material/styles
since we want to useuseTheme
Which ideally would be eventually replaced with the @mui/system
import if there were no issues with theme merging/scoping. 🤔 🤞
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.
Yes, not sure when this would be though
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Closes #12277
This PR will remain opened until
@mui/material
reaches its v6 stable release and Pigment reaches its v1 stable release.By then we should have a better view of what we want to do.