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

[pickers] Use the new ownerState in the toolbar components #15777

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 6, 2024

Part of #14475

@flaviendelangle flaviendelangle self-assigned this Dec 6, 2024
@mui-bot
Copy link

mui-bot commented Dec 6, 2024

Deploy preview: https://deploy-preview-15777--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against ab2def7

const slots = {
root: ['root', selected && 'selected'],
root: ['root'],
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'm dropping this state class in favor of a data attribute.
We will probably drop most of them since Base UI (and therefore @mui/material) is migrating toward data attributes. And dropping this one allowed me to not create a custom ownerState for this component...

* If this context value is set to true, the toolbar will always be rendered in the desktop mode.
* This is used by the Date Time Range Picker Toolbar.
*/
export const DateTimePickerToolbarForceDesktopVariant = React.createContext(false);
Copy link
Member Author

@flaviendelangle flaviendelangle Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to remove the hack of overriding toolbarVariant to enforce desktop layout on the DateTimeRangePickerToolbar component.

<DateTimePickerToolbarTimeContainer
className={classes.timeContainer}
ownerState={ownerState}
toolbarVariant={toolbarVariant}
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'm passing this as a prop to avoid creating a dedicated ownerState for DateTimePickerToolbar

* Is equal to "ltr" when the toolbar is in left-to-right direction.
* Is equal to "rtl" when the toolbar is in right-to-left direction.
*/
toolbarDirection: 'ltr' | 'rtl';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #14994 I went for isRtl, but I think I would prefer the more descriptive {componentName}Direction instead.

I can update the PickerLayoutOwnerState to use layoutDirection as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flaviendelangle flaviendelangle force-pushed the toolbar-clean-ownerState branch from 6c6fd46 to c6b8611 Compare December 6, 2024 15:09
@flaviendelangle flaviendelangle force-pushed the toolbar-clean-ownerState branch from 0baf534 to 017d97e Compare December 6, 2024 15:33
@flaviendelangle flaviendelangle added component: pickers This is the name of the generic UI component, not the React module! breaking change labels Dec 6, 2024
@flaviendelangle flaviendelangle marked this pull request as ready for review December 6, 2024 16:03
Copy link
Member

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really nice work ... LGTM :shipit:

@flaviendelangle flaviendelangle merged commit 7eccbd0 into mui:master Dec 9, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the toolbar-clean-ownerState branch December 9, 2024 15:20
LukasTy pushed a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants