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 usePickerContext() and usePickerActionsContext() instead of passing props to the shortcuts and toolbar slots #15948

Merged
merged 3 commits into from
Dec 30, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 19, 2024

Closes #15495 ( 🙏 )

What?

  • Remove the last props passed to the shortcuts, layout and toolbar slots
  • Introduce a new useIsValidValue hook to avoid passing something to usePickerContext that updates on every render
  • Add value to usePickerContext
  • Add setValue to usePickerContext and usePickerActionContext. This should eventually be the only way for parts of the UI to update the value (it will replace the onChange prop passed to the views and to the field in the future).
  • Rename PickerShortcutChangeImportance into PickerChangeImportance since it's not only used for the shortcut anymore.

Changelog

Breaking changes

  • The component passed to the layout slot no longer receives the value, onChange and onSelectShortcut props — Learn more.

  • The component passed to the toolbar slot no longer receives the value, onChange and isLandscape props — Learn more.

  • The component passed to the shortcuts slot no longer receives the onChange, isValid and isLandscape props — Learn more.

  • The PickerShortcutChangeImportance type has been renamed PickerChangeImportanceLearn more.

@flaviendelangle flaviendelangle added breaking change component: pickers This is the name of the generic UI component, not the React module! labels Dec 19, 2024
@flaviendelangle flaviendelangle self-assigned this Dec 19, 2024
@mui-bot
Copy link

mui-bot commented Dec 19, 2024

Localization writing tips ✍️

Seems you are updating localization 🌍 files.

Thank you for contributing to the localization! 🎉 To make your PR perfect, here is a list of elements to check: ✔️

  • Verify if the PR title respects the release format. Here are two examples (depending if you update or add a locale file)

    [l10n] Improve Swedish (sv-SE) locale
    [l10n] Add Danish (da-DK) locale

  • Update the documentation of supported locales by running pnpm l10n
  • Clean files with pnpm prettier.

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

Updated pages:

Generated by 🚫 dangerJS against 361fb4a

@flaviendelangle flaviendelangle force-pushed the layout-last-props branch 4 times, most recently from 2f4fb12 to 6fe5cfa Compare December 20, 2024 10:32
@flaviendelangle flaviendelangle changed the title [pickers] Remove the last props passed to the layout slot [pickers] Use usePickerContext() and usePickerActionsContext() to get the propspassed to the shortcuts and toolbar slots Dec 20, 2024
@flaviendelangle flaviendelangle changed the title [pickers] Use usePickerContext() and usePickerActionsContext() to get the propspassed to the shortcuts and toolbar slots [pickers] Use usePickerContext() and usePickerActionsContext() instead of passing props to the shortcuts and toolbar slots Dec 20, 2024
@flaviendelangle flaviendelangle force-pushed the layout-last-props branch 3 times, most recently from 59ca6f1 to bfb0786 Compare December 20, 2024 11:24
/**
* Returns a function to check if a value is valid according to the validation props passed to the parent picker.
*/
export function useIsValidValue<TValue extends PickerValidValue>() {
Copy link
Member Author

@flaviendelangle flaviendelangle Dec 20, 2024

Choose a reason for hiding this comment

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

For now it's a super dumb hook.
It re-renders everytime the picker render and just calls the function that the picker gives him (which calls the validation).

But introducing this hook will allow us in the future to add it to the Date Calendar and Digital Clock as well and potentially to add some caching mechanism to avoid testing several times the same date.

For now it's only needed for the shortcut slot because we were passing this isValid prop.
When the view and the field will use it, it will probably impact the shape of the context(s) related to validation, but I'm pretty sure we will still have a hook with this exact signature 👌

@flaviendelangle flaviendelangle marked this pull request as ready for review December 20, 2024 11:53
* The shortcut that triggered this change.
* Should not be defined if the change does not come from a shortcut.
*/
shortcut?: PickersShortcutsItemContext;
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 the Base UI version we won't have a notion of shortcut, so this field will probably only live in the MUI version (since some people needed it).

The other will be handled by the Base UI layer.

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.

Looks good i general ... only concern is that we could simplify all places where we check for value == null || !utils.isValid(value) into utils.isValid(value) ... agree?

@flaviendelangle flaviendelangle force-pushed the layout-last-props branch 2 times, most recently from 0617646 to 3befa0c Compare December 30, 2024 08:27
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.

Only nits... feel free to ignore... I love the cleanup! Much more readable now! Thanks for taking care of it! 🙇

@flaviendelangle flaviendelangle merged commit 61346b3 into mui:master Dec 30, 2024
18 checks passed
@flaviendelangle flaviendelangle deleted the layout-last-props branch December 30, 2024 09:19
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.

[pickers] Remove all the props passed by usePickers to PickersLayout and use the new contexts instead
3 participants