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 new DigitalClock desktop time picking experience #7958

Merged
merged 164 commits into from
Apr 27, 2023

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Feb 16, 2023

Fixes #4483

Preview: https://deploy-preview-7958--material-ui-x.netlify.app/x/react-date-pickers/time-picker/

new-time-picker-ui

Changes

  • Add option to render "digital clock" using "select list"
  • Support digital view renderer on DesktopTimePicker and DesktopDateTimePicker
  • Add desktop time views renderer
  • Update docs
  • Update tests
  • Add a11y support for MultiSectionDigitalClock and DigitalClock
  • Add new tests

Open compromise questions for initial implementation

  1. Do we ship the digital clock renderer only on TimePicker and leave DateTimePicker for a later time/PR after the design has been finalized and DateTimeRange behavior is possibly explored and decided on?
  2. Do we add the new props to MobileTimePicker? If we don't finalize usage of the DigitalClock on mobile, then maybe it makes sense to not have the props on that component as well? But no one would stop users from specifying the new digital timeViewRenderers for the MobileTimePicker, hence, some users might be in a problem, where they insist on using the renderer, but TS does not allow for the new expected props to be passed to it... 🤷
  3. Do we keep the StaticTimePicker behavior as is (using TimeClock) for the time renderer regardless of the displayStaticWrapperAs prop value?
  4. What kind of default behavior do we want for select/closing?
    1. Should the time picker default to having an accept button and not close by default after selection (unless closeOnSelect is true)?
    2. Should the time picker have the same defaults—close by default after all sections are selected? In this case we either need a meridiem view or ugly code to handle ampm="true" case.
  5. Wait for all the little design/UX (colors, scrolling after selecting, etc.) questions that are being looked at by Gerda.

My answers to questions:

  1. Yes, ship implementation only for the TimePicker initially.
  2. I'm really not sure on this one but would lean toward omitting the props from the mobile picker at least for the initial release.
  3. Yes, keep what we currently have.
  4. I lean towards going for the second option regardless of how ugly the implementation would be. 🙈
  5. This is somewhat sensitive. I'd be in favor of shipping more or less the final product UX-wise, without any significant changes in the following releases. 🙈

What are your opinions on these questions @flaviendelangle @joserodolfofreitas @alexfauquette?

@LukasTy LukasTy added component: pickers This is the name of the generic UI component, not the React module! component: DateTimePicker The React component. component: TimePicker The React component. labels Feb 16, 2023
@LukasTy LukasTy self-assigned this Feb 16, 2023
@mui-bot
Copy link

mui-bot commented Feb 16, 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.

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-7958--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 671.8 1,401.2 671.8 961.98 264.501
Sort 100k rows ms 733.9 1,512.9 1,512.9 1,172.32 298.897
Select 100k rows ms 299.6 322.7 317.5 313.74 8.854
Deselect 100k rows ms 159.3 396.6 216.9 247.84 84.106

Generated by 🚫 dangerJS against a927e81

@LukasTy LukasTy added the new feature New feature or request label Feb 16, 2023
@LukasTy LukasTy changed the title [pickers] Add desktop time picking experience [pickers] Add new desktop time picking experience Feb 16, 2023
@LukasTy
Copy link
Member Author

LukasTy commented Feb 27, 2023

@gerdadesign Could you have a look at the live time picker and date time picker examples and comment on the UX behavior?

The main I see:

  • Should the meridiem selection be in the selectable views flow?
    • should we only close the picker after it is changed/confirmed?
    • current rough implementation uses a similar approach to the mobile (v5) clock picker, where am/pm toggle is independent between the picker lifecycle—if you did not change it before selecting the last section (minutes by default)—the picker will be closed
  • Or should we go with what is currently in the examples: closing the picker when the last section is selected (if a value is already present—also close when selecting the last section right after opening the picker without selecting the prior section)?
  • Should we support these new views on mobile and static pickers?
  • Should we handle toolbar behavior on the digital (select options) picker? And if yes? How? Would clicking time sections do nothing?

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

github-actions bot commented Mar 1, 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 Apr 26, 2023
@@ -1,10 +1,12 @@
import { PickersLocaleText } from './utils/pickersLocaleTextApi';
import { getPickersLocalization } from './utils/getPickersLocalization';
import { TimeViewWithMeridiem } from '../internals/models';
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think that it still makes sene in keeping this type internal if we are adding it into locale files? 🤔

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

This pull request has conflicts, please resolve those before we can evaluate the pull request.

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.

Nothing to add, great work ! 🥳

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

@joserodolfofreitas joserodolfofreitas left a comment

Choose a reason for hiding this comment

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

Nice work, @LukasTy!
I really appreciate the effort in pushing the boundaries on polishment and customization-abilities with these components.

It looks and feels great, and just looking at the sheer number of files this PR required to change and create is a signal of how big this task was. All the kudos!

There're always improvements to be done, but I believe none of them should block the release. I'll create issues for them as follow-ups.

  • The auto-closing strategy might still need some tweaks, but we can try improving it after user validation. I think that handling the last selector as an "OK" button may create some unwanted situations.

  • Maybe it's good to have at least one visual customization example, like, changing the columns or action buttons design. Does it have any particular slots?

On an unrelated note, the readOnly examples for the pickers could display a value.


You can force a desired interval using the `timeStep` prop.

{{"demo": "DigitalClockTimeStep.js"}}
Copy link
Member

Choose a reason for hiding this comment

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

On one side, it makes a lot of sense to have different properties since they ultimately produce a different behavior.

But I imagine this will likely create some confusion/friction for the users, which makes me realize we aren't releasing these new components as unstable, so changing its API before v7 can become a pain.

I wish we could somehow warn the user they are using the wrong property. Based on type only, the IDE will only say that DigitalClock doesn't have timeSteps property and vice versa.

@LukasTy
Copy link
Member Author

LukasTy commented Apr 27, 2023

On an unrelated note, the readOnly examples for the pickers could display a value.

That is a good idea. It was not that big of an issue until we introduced the placeholder/value behavior. 🙈

@LukasTy LukasTy changed the title [pickers] Add new desktop time picking experience [pickers] Add new DigitalClock desktop time picking experience Apr 27, 2023
@LukasTy LukasTy merged commit 0cb20a0 into mui:master Apr 27, 2023
@LukasTy LukasTy deleted the digital-clock branch April 27, 2023 15:57
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! component: TimePicker The React component. new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] Replace clock on desktop from TimePicker
7 participants