-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Make changeImportance
and shortcut
mandatory in PickersShortcuts
#10941
Conversation
Deploy preview: https://deploy-preview-10941--material-ui-x.netlify.app/ Updated pages: |
38bf339
to
d60bafb
Compare
changeImportance
and shortcut
mandatory in PickersShortcut
changeImportance
and shortcut
mandatory in PickersShortcut
changeImportance
and shortcut
mandatory in PickersShortcuts
ee4d52d
to
b45dae7
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@LukasTy @alexfauquette that one may have disappeared in your notifications |
- The [`changeImportance`](/x/react-date-pickers/shortcuts/#behavior-when-selecting-a-shortcut) of the shortcut. | ||
- The `item` containing the entire shortcut object. |
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 assume not providing the changeImportance
will lead to broken shortcuts.
But for the item
I'm not sure about what's the impact.
If I correctly understand, the usage of item
is to provide some context to the onChange
. What do you think about saying the last parameter is the shortcut context? Such that devs know they can pass whatever they think valuable for the onChange
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.
If I correctly understand, the usage of item is to provide some context to the onChange
Yes, it's just passed directly to context.shortcut
.
What do you think about saying the last parameter is the shortcut context? Such that devs know they can pass whatever they think valuable for the onChange
I guess it's unlikely that we will ever use this value for something else than just forwarding it to the context, so people should be able to pass whatever they want indeed.
Would you agree that it's not the role of the migration guide though? More the role of the shortcut doc if we want to highlight this ability at some point.
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, it's just that it's the only place where this feature is explained for now
changeImportance
and shortcut
mandatory in PickersShortcuts
changeImportance
and shortcut
mandatory in PickersShortcuts
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.
LGTM. 👌
docs/data/migration/migration-pickers-v6/migration-pickers-v6.md
Outdated
Show resolved
Hide resolved
docs/data/migration/migration-pickers-v6/migration-pickers-v6.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Lukas <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>
Co-authored-by: Lukas <llukas.tyla@gmail.com> Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>
Closes #10940