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 DateTimeRangePicker component #9528

Merged
merged 178 commits into from
Jan 24, 2024

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Jun 30, 2023

Fixes #4547
Preview: https://deploy-preview-9528--material-ui-x.netlify.app/x/react-date-pickers/date-time-range-picker/
This will be release as stable in MUI X v7

Note

The PR is too big as it is, I'll either wait with the changes on validation and find your component pages or maybe keep those for a separate PR. 🤔

The current implementation uses rendererInterceptor idea, which renders a 2-column layout on the desktop and wraps the provided timeViewRenderer so that the existing renderers can be re-used and would work with a [start, end] value.

Pros of this approach

  • Allows reusing existing timeViewRenderers;
  • Smaller bundle, because no extra dateTimeRangeViewRenderers are created;
  • Overriding the time view renderer is very straightforward for the user;

Cons of this approach

  • Possibly a few more re-renders (re-mounts);
  • Changing from the proposed 2-column layout on the desktop is nearly impossible;

Example of how to change specific view rendering

https://deploy-preview-9528--material-ui-x.netlify.app/x/react-date-pickers/date-time-range-picker/#change-view-renderer

Middleground

We can decide to reuse the same logic as with DesktopDateTimePicker and just create a dateTimeRangeViewRenderers and "call it a day". 🤔

TODO

  • Add tests
  • Add themeAugmentation and spec test
  • Add the component to the Validation page
  • Add the component to Find your component section

Changelog

@LukasTy LukasTy added plan: Pro Impact at least one Pro user component: pickers This is the name of the generic UI component, not the React module! labels Jun 30, 2023
@LukasTy LukasTy self-assigned this Jun 30, 2023
@mui-bot
Copy link

mui-bot commented Jun 30, 2023

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 yarn l10n
  • Clean files with yarn prettier.

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

Updated pages:

Generated by 🚫 dangerJS against 2bd2bfb

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 5, 2023
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

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 Jul 5, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 12, 2023

This looks like a great start 👍


I have added more benchmarks in 🚤 Date Time Range Picker component for this component.


To comment on https://app.slack.com/client/TS83SPSTV/C055FMZKB28/thread/C055FMZKB28-1689256302.407199?cdn_fallback=2, I have noticed this:

  1. A bug, there is a strange auto-close logic. Notice how the selection moves incrementally to the end to eventually auto close (I'm selected random date ranges):
Screen.Recording.2023-07-13.at.21.55.59.mov
  1. The "start" "end" buttons got me confused, it took me a few tries to understand what they do.
Screenshot 2023-07-13 at 21 53 20

I feel that

Screenshot 2023-07-13 at 21 53 11

is a better use of space, or honestly blank might be even better:

Screenshot 2023-07-13 at 21 58 15
  1. I like how Ant Design gives you a clue about which input is selected in the calendar:
Screenshot 2023-07-13 at 21 46 04

Maybe there is a way to communicate this as well.

@oliviertassinari oliviertassinari added the new feature New feature or request label Jul 13, 2023
@LukasTy
Copy link
Member Author

LukasTy commented Jul 14, 2023

@oliviertassinari Thank you for the feedback! 🙏

The "start" "end" buttons got me confused, it took me a few tries to understand what they do.

We've added those as an option to toggle between selecting start or end time selection. AntD does not seem to provide an option to do it via UI, only the inputs. 🤷
cc @gerdadesign

  1. I like how Ant Design gives you a clue about which input is selected in the calendar:

Don't we? Or am I missing something? 🤔 Could you pint point or clarify what are you actually referring to? 🤔

@oliviertassinari
Copy link
Member

  1. Don't we? Or am I missing something? thinking Could you pint point or clarify what are you actually referring to? thinking

When only looking at the popup's content, you can't determine if you are on the start or the end of the range. You have to look a bit up to check the input focus style.

@LukasTy
Copy link
Member Author

LukasTy commented Jul 15, 2023

When only looking at the popup's content, you can't determine if you are on the start or the end of the range. You have to look a bit up to check the input focus style.

By clue, do you mean the fact, that they disable the days before the range start if the end input is focused? 🤔
If that's the case, then they don't support dragging, hence, it's not a problem for them, but we do support dragging and dragging the end date before the start date, which flips dates...

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 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 Jan 24, 2024
@LukasTy
Copy link
Member Author

LukasTy commented Jan 24, 2024

Requesting a final re-review from @flaviendelangle.
P.S. I'll try to squeeze TODO items 2-4 before the release via separate PR(s).
Did not want to further increase the size of this one...

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Thank you so much for all the work around this topic ❤️

@LukasTy LukasTy merged commit e8844c9 into mui:next Jan 24, 2024
17 checks passed
@LukasTy LukasTy deleted the add-date-time-range-picker branch January 24, 2024 22:23
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Great 👍

@DanailH
Copy link
Member

DanailH commented Jan 26, 2024

Great to see this being merged. Half a year of solid effort 🚀 Thank you @LukasTy!

@headironc
Copy link
Contributor

👍🏻 Thank you for your effort!

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! new feature New feature or request plan: Pro Impact at least one Pro user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] New component: DateTimeRangePicker
7 participants