-
-
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] Remove orientation
, isLandscape
, isRtl
, wrapperVariant
and disabled
props from PickersLayout
#15494
[pickers] Remove orientation
, isLandscape
, isRtl
, wrapperVariant
and disabled
props from PickersLayout
#15494
Conversation
04fa74d
to
e5d2772
Compare
Deploy preview: https://deploy-preview-15494--material-ui-x.netlify.app/ Updated pages: |
…disabled props from PickersLayout
e5d2772
to
6fb5cef
Compare
@@ -34,8 +34,8 @@ You can override the actions displayed by passing the `actions` prop to the `act | |||
actions: ['clear'], | |||
}, | |||
// The actions will be different between desktop and mobile | |||
actionBar: ({ wrapperVariant }) => ({ | |||
actions: wrapperVariant === 'desktop' ? [] : ['clear'], | |||
actionBar: ({ variant }) => ({ |
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 will try to write a first migration guide for the new ownerState
now that we migrated a good portion of the components.
This will be done in a follow up 👍
</LocalizationProvider> | ||
<iframe | ||
title="codesandbox" | ||
src="https://codesandbox.io/embed/https-mui-com-x-migration-migration-pickers-v7-forked-9fz6f6?hidenavigation=1&fontsize=14&view=preview" |
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'm breaking APIs that were used in the demo, and we want this demo to still use only v6 API,s not v8 APIs.
The layout is not great
It's the same as in https://mui.com/x/migration/migration-data-grid-v4/#using-mui-core-v4-with-v5 and since it's an old migration guide I think it's sufficient
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.
This feels fine to me 👌
@@ -276,6 +289,48 @@ const theme = createTheme({ | |||
+console.log(readOnly); | |||
``` | |||
|
|||
- The component passed to the `layout` slot no longer receives a `isRtl` prop. If you need to access this information, you can use the `useRtl` hook from `@mui/system`: |
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.
Alternatively I can suggest to use ownerState.isRtl
but I think that's not great.
In any case, probably nobody uses this prop...
|
||
const classes = useUtilityClasses(props); | ||
const classes = useUtilityClasses(classesProp, ownerState); |
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.
It's weird that we are resolving the props both in usePickerLayout
and in PickersLayout
But I won't fix that now since we will probably rework the whole DX
@@ -90,7 +82,10 @@ export const usePicker = < | |||
shouldRestoreFocus: pickerViewsResponse.shouldRestoreFocus, | |||
|
|||
// Picker layout | |||
layoutProps: pickerLayoutResponse.layoutProps, | |||
layoutProps: { |
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 will do a follow up to remove those props as well 😆
Most of them are forwarded directly to the slots handled by the layout so I will probably edit those slots first.
But the goal to have PickersLayout
receive 0 prop outside of slotProps.layout
is clearly achievable 👌
And it should be doable as well for all the slots it handles but potentially with better DX evolution so I don't think it will be done for v8 and it might require a deprecation phase.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
86357d2
to
e98f435
Compare
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 improvement, LGTM👌 🚀
</LocalizationProvider> | ||
<iframe | ||
title="codesandbox" | ||
src="https://codesandbox.io/embed/https-mui-com-x-migration-migration-pickers-v7-forked-9fz6f6?hidenavigation=1&fontsize=14&view=preview" |
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.
This feels fine to me 👌
Part of #15495
Extracted from #15300
Follow up on #15492