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] Pass the shortcut information in the onChange context #9985

Merged
merged 11 commits into from
Aug 11, 2023

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Aug 10, 2023

Closes #9897

@alexfauquette I would like your opinion on this one.
Would you pass the label?
Would you pass the entire shortcut object?
Would you go for the more hacky solution proposed in the issue of adding an onChange prop to the shortcut object? (I fear that people trigger there onChange based on this slot prop instead of using the picker props).
Another approach?
Do nothing?

@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Aug 10, 2023
@flaviendelangle flaviendelangle self-assigned this Aug 10, 2023
@flaviendelangle flaviendelangle changed the title [pickers] Pass the shortcut label in onChange context [pickers] Pass the shortcut label in the `onChange_ context Aug 10, 2023
@flaviendelangle flaviendelangle changed the title [pickers] Pass the shortcut label in the `onChange_ context [pickers] Pass the shortcut label in the onChange context Aug 10, 2023
@mui-bot
Copy link

mui-bot commented Aug 10, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9985--material-ui-x.netlify.app/

Updated pages

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 319.2 560.1 405.2 416.12 87.635
Sort 100k rows ms 762.5 1,443.6 1,412 1,222 247.915
Select 100k rows ms 674.4 778.1 731.8 731.22 39.315
Deselect 100k rows ms 116.3 239.2 147.9 169.18 44.148

Generated by 🚫 dangerJS against dca3b2d

@alexfauquette
Copy link
Member

I would be in favor of passing an object. For example shortcutContext because it's only label and getValue but adding an id could be a nice thing. to support l10n.

For example, new month would be

{
  id: 'next-month',
  label: t('pickers.shortcuts.next-month'),
  getValue: () => ...
}

And I assume we could get other requests

@flaviendelangle
Copy link
Member Author

I agree that passing the full object would be more future-proof

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Aug 10, 2023

PR updated
I'm not passing getValue because I think it would be an anti-pattern to use it (the value returned by it has not been post-processed by usePickerValue)

@LukasTy LukasTy changed the title [pickers] Pass the shortcut label in the onChange context [pickers] Pass the shortcut information in the onChange context Aug 10, 2023
@LukasTy LukasTy added the enhancement This is not a bug, nor a new feature label Aug 10, 2023
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice improvement! 👍

docs/data/date-pickers/shortcuts/shortcuts.md Outdated Show resolved Hide resolved
packages/x-date-pickers/src/models/pickers.ts Outdated Show resolved Hide resolved
docs/data/date-pickers/shortcuts/OnChangeShortcutLabel.tsx Outdated Show resolved Hide resolved
docs/data/date-pickers/shortcuts/OnChangeShortcutLabel.tsx Outdated Show resolved Hide resolved
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Looking great! 💙
Just though a possibly very useful and relevant addition for the demo.
What do you think about marking the shortcut chip that has been selected as active?
I imagine that such change might be needed by most people caring about, which shortcut has been clicked. 🤔
This could uncover the fact that it's pretty much mandatory to add an id prop to shortcut item in order to reliably identify, which shortcut has actually been called.

Imagine a case of multi locale application having translated shortcuts, user selects a shortcut and it is supposedly saved in storage. Once the user visits the page again having changed locale or maybe the shortcut label (translation) has been updated—the saved shortcut can no longer be mapped. 🙈

packages/x-date-pickers/src/models/pickers.ts Outdated Show resolved Hide resolved
Co-authored-by: Lukas <llukas.tyla@gmail.com>
Signed-off-by: Flavien DELANGLE <flaviendelangle@gmail.com>
@flaviendelangle
Copy link
Member Author

This could uncover the fact that it's pretty much mandatory to add an id prop to shortcut item in order to reliably identify, which shortcut has actually been called.

The only issue here is that the type of the shortcut is fixed, you can not pass it as a generic to your component.
We would need to add a as in the demo to make it work.
So adding the id in the demo would help for the reasons you explained but would make the demo a little more hacky.


For the idea of the chip, that would be a nice improvement indeed 👌

@flaviendelangle
Copy link
Member Author

Looks like the individual shortcut are not a slot, so it's hard to pass props to them.
Maybe we could improve the demo as a follow up if we want to create a new slot.

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Looks like the individual shortcut are not a slot, so it's hard to pass props to them.
Maybe we could improve the demo as a follow up if we want to create a new slot.

It's nice that our investigation uncovered a shortcoming in the implementation. 👍

@flaviendelangle
Copy link
Member Author

@alexfauquette I let you confirm that I'm not missing a way to do this customization with the current API and I will create an issue if not 👍

@flaviendelangle flaviendelangle merged commit 7ad4658 into mui:master Aug 11, 2023
4 checks passed
@flaviendelangle flaviendelangle deleted the shortcut-label branch August 11, 2023 13:44
@alexfauquette
Copy link
Member

@alexfauquette I let you confirm that I'm not missing a way to do this customization with the current API and I will create an issue if not 👍

I confirm it's not feasible, at least not in an easy way

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! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[question] [DateRangePicker] Knowing which shortcut was clicked / accepted
4 participants