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] Add new properties to PickerOwnerState and PickerContextValue #15415

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Nov 14, 2024

Extracted from #15300
Part of #14475
Part of #14718

In this PR

  • Rename wrapperVariant into variant.
    I always found this name confusing. What is the "wrapper"? For now wrapperVariant is only public in some ownerState and as a prop received by PickersLayout (and the toolbars have an equivalent prop named toolbarVariant), but if we add it to the public context and to all the ownerState, we will make it widely used so I think it's important to think about its naming now.
    Side note: I did not rename them on any public API since the goal is to remove them and use the context instead.

  • Add variant, orientation, disabled and readOnly to the PickerContextValue interface (accessed via usePickerContext).
    I want to stop passing props to PickersLayout and to the toolbar components to prepare for the composition API so I need another way to access them. I'm adding it to the public context not to the private one because when creating a custom toolbar people currently have access to this info (through the props) and should continue to have access to it.

  • Add pickerVariant and pickerOrientation to the PickerOwnerState interface.
    I was relunctant to do it, but while working on [pickers] ON HOLD - Use the new ownerState in all the toolbars and stop passing variant and isLandscape to any component #15300 it was clear that we have a lot of slots (especially styled() components) that uses it and it's a pain to manually add it everytime.

I went for orientation / pickerOrientation instead of isLandscape because I think it provides a better DX and if we change how people access this information, it's also the right time to change the value. But I can revert to isLandscape / isPickerInLandscape instead if you prefer.

@flaviendelangle flaviendelangle self-assigned this Nov 14, 2024
@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Nov 14, 2024
@flaviendelangle flaviendelangle force-pushed the variant-orientation-context branch from 0c9c094 to 6b86c85 Compare November 14, 2024 10:37
extends Pick<PickerProviderProps, 'localeText'> {
pickerValueResponse: UsePickerValueResponse<TValue, FieldSection, any>;
ownerState: PickerOwnerState;
function getOrientation(): PickerOrientation {
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic of getOrientation and usePickerOrientation comes from useIsLandscape which is used in usePickerLayoutProps and will be removed once we stop passing isLandscape as a prop to PickersLayout

@mui-bot
Copy link

mui-bot commented Nov 14, 2024

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

Generated by 🚫 dangerJS against 4d04b0c

@flaviendelangle flaviendelangle force-pushed the variant-orientation-context branch from 6b86c85 to e82a694 Compare November 14, 2024 10:41
@flaviendelangle flaviendelangle changed the title [pickers] Add the variant and the orientation to the ownerState and to usePickerContext [pickers] Add properties to PickerOwnerState and PickerContextValue Nov 14, 2024
@flaviendelangle flaviendelangle force-pushed the variant-orientation-context branch from e82a694 to ea217ec Compare November 14, 2024 10:55
*/
export interface UsePickerProviderProps extends FormProps {
/**
* Force rendering in particular orientation.
Copy link
Member Author

@flaviendelangle flaviendelangle Nov 14, 2024

Choose a reason for hiding this comment

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

It's the JSDoc we currently have in usePickersLayoutProps but it's probably not great 🤔
If you have a better one, I can apply it in a follow up (to avoid poluting this PR with all the doc gen)

Copy link
Member

Choose a reason for hiding this comment

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

I would go with something like "Specifies the picker's orientation"

@flaviendelangle flaviendelangle force-pushed the variant-orientation-context branch from ea217ec to b27b009 Compare November 14, 2024 10:58
@flaviendelangle flaviendelangle changed the title [pickers] Add properties to PickerOwnerState and PickerContextValue [pickers] Add new properties to PickerOwnerState and PickerContextValue Nov 14, 2024
@flaviendelangle flaviendelangle marked this pull request as ready for review November 14, 2024 11:14
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 15, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 15, 2024
Copy link
Member

@arthurbalduini arthurbalduini left a comment

Choose a reason for hiding this comment

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

LGTM 🎉
The new naming makes sense in my opinion. Thanks for taking care of it

@flaviendelangle flaviendelangle merged commit a4481b6 into mui:master Nov 19, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the variant-orientation-context branch November 19, 2024 13:52
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
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