-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 TimeRangePicker
component
#9431
base: master
Are you sure you want to change the base?
Conversation
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: ✔️
Deploy preview: https://deploy-preview-9431--material-ui-x.netlify.app/ Updated pages: |
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.
Leaving some minor observations here. 🙈
|
||
const props = { | ||
...defaultizedProps, | ||
// TODO: Do we want the toolbar to support AM/PM switch on mobile? |
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.
Probably no, unless we go with the AnalogClock
on Mobile. 🤔
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.
So the plan is to have the digital clock on mobile for range pickers and the analog clock on mobile for TimePicker
/ DateTimePicker
?
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 we'd go with the proposed designs - it would seem so, but that is still up for debate. 🙈
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.
Is the component ready for mobile?
I'm 100% in favor of doing it. But if we do it, for me it means that in the future we will use the digital clock on the MobileTimePicker
.
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.
Is the component ready for mobile?
It had some issues, which I tried addressing in the DateTimeRangePicker PR.
I'm 100% in favor of doing it. But if we do it, for me it means that in the future we will use the digital clock on the
MobileTimePicker
.
That part we still need to align on the Design and Product side. I'm also for consistency, but we currently do not seem to have a singular position that everyone agrees on. 🙈
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 think we can focus our next Monday meeting on this topic 👌
Concerning the rendererInterceptor
, it's not easy to understand what is the goal but I'll wait to have the JSDoc to have a 2nd look 👍
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 think we can focus our next Monday meeting on this topic 👌
Oh yes, that's one of the last pressing design matters for the range pickers.
Concerning the
rendererInterceptor
, it's not easy to understand what is the goal but I'll wait to have the JSDoc to have a 2nd look 👍
Don't pay too much attention to it. That was my alpha implementation for configurable view renderer support and handling view combination (DateRangeCalendar
on the left and MultiSectionDigitalClock
or any other timeViewRenderer
on the right).
packages/x-date-pickers-pro/src/DesktopTimeRangePicker/DesktopTimeRangePicker.tsx
Outdated
Show resolved
Hide resolved
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Doc preview
Closes #4460