[pickers] Fix am/pm buttons position and responsiveness #5149

alexfauquette · 2022-06-08T15:05:09Z

Fix #4947
Fix #7279
Second part of #4446

Explanation commit by commit:

ampmInClock is used to know if buttons are in there clock or the toolbar (not the appbar). So I rewrite the ampmInClock behavior as follow:
- If there is no toolbar, ampm buttons are in the clock whatever is the value of ampmInClock. Otherwise, end-users would not have access to ampm
- If there is a toolbar, ampm buttons are either in the toolbar or the clock (not both) according to ampmInClock
I forgetted to propagate ampmInClock to the ToolbarComponent
MobileDateTimePicker neither display the toolbar eithen if showToolbar is set to true fixing that was asked in [pickers] getViewSwitchingButtonText prop does not work #4446
Add ampm control in the toolbar of MobileDateTimePicker
Inverse the order between components such that the view switcher is over the clock which is stealing the click

Before: with code | without code
After: with code | without code

Changelog

On desktop, DateTimePicker shows the ampm controls in the toolbar instead of the clock by default.
It can be overridden by specifying ampmInClock prop.

mui-bot · 2022-06-08T15:14:08Z

Netlify deploy preview

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

Updated pages

No updates.

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	707.3	1,155.8	707.3	943.84	188.625
Sort 100k rows	ms	631.9	1,075.2	811.7	869.12	149.216
Select 100k rows	ms	187.1	283.3	228.1	232.38	30.932
Deselect 100k rows	ms	151.2	306.1	186	202.58	56.723

Generated by 🚫 dangerJS against 1addb5f

alexfauquette · 2022-06-09T09:39:10Z

It still has one problem: the pen icon in the toolbar is at the same location as the view switcher of the clock

flaviendelangle · 2022-07-21T15:27:47Z

On the DateTimePicker (both mobile and desktop) with both props true.

alexfauquette · 2022-07-25T07:32:17Z

Strange because I do not manage to reproduce this issue which looks like an encoding one

github-actions · 2022-09-01T12:51:19Z

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

alexfauquette · 2023-01-04T13:10:44Z

packages/x-date-pickers/src/DateTimePicker/DateTimePickerToolbar.tsx

+    ampmSelection: ['ampmSelection'],
+    ampmLabel: ['ampmLabel'],


This naming (like most of the code) is taken from TimePickerToolbar

github-actions · 2023-01-09T13:52:41Z

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

alexfauquette · 2023-01-09T15:49:07Z

To test all to configuration I made a demo:

https://codesandbox.io/s/nervous-robinson-05gnne?file=/demo.tsx

To compare with the same demo based on the latest version
https://codesandbox.io/s/gracious-edison-qs4mn8

LukasTy

Great job! 🚀
Such an annoying issue finally fixed. 💪

I couldn't find any issues testing what I could think of. 😉

packages/x-date-pickers/src/DateTimePicker/dateTimePickerToolbarClasses.ts

...ages/x-date-pickers/src/internals/components/CalendarOrClockPicker/CalendarOrClockPicker.tsx

LukasTy · 2023-01-16T17:30:08Z

packages/x-date-pickers/src/internals/hooks/usePicker/usePickerViews.ts

+   * If `true`, the toolbar will be visible.
+   * @default `true` for mobile, `false` for desktop
+   */
+  showToolbar?: boolean;


Heads-up: This is going to conflict with #7498

An initial idea was to show view switcher only if:

There is view to swich (more than 1 time view)

The swich is not already available in the toolbar

Since showToolbar is not available any more, and using slotProps.toolbar.hidden does not seem reliable (as a developer it would be strange that a slot props impact another one) I propose to go to the safest way: show the view swich even if there is a toolbar. Leading to the following argos diff

@LukasTy does it seems good for you?

Since showToolbar is not available any more, and using slotProps.toolbar.hidden does not seem reliable (as a developer it would be strange that a slot props impact another one)

Hm. Why do you think like that? showToolbar was being used to understand if this view switcher need to be shown, but the only thing that changed is the location this prop has moved to.
If we want/need the view switcher logic to depend on toolbar prop, I do not see much problem with it. 🤔
Maybe adding a mention about this relationship somewhere would be useful, but I don't know the best place for it. 🙈

Note that this kind of scenario won't work well if we support the callback format at somepoint.
Because we would not have the ownerState outside of the layout component, so we could not correctly resolve the toolbar.hidden prop.

Yes, I did not thaught about that edge case

github-actions · 2023-01-17T09:01:41Z

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

github-actions · 2023-01-17T10:47:17Z

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

github-actions · 2023-01-17T17:07:06Z

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

LukasTy

Looking great! 👌
Had another go after all the recent changes in API and couldn't find issues with it.
Looks like a great improvement and fixes the shortcoming of not having meridiem switcher in DateTimePickerToolbar. 🙌

There are a few final issues to solve before we could merge this. 😉

FYI. Been using this sandbox as well as locale changes.

P.S. The PR title could probably benefit from an update as the PR does impact more than it mentions. 🙈

packages/x-date-pickers/src/DateTimePicker/DateTimePickerToolbar.tsx

LukasTy · 2023-03-02T15:29:41Z

packages/x-date-pickers/src/DateTimePicker/DateTimePickerToolbar.tsx

+            <PickersToolbarButton
+              disableRipple
+              variant="subtitle2"
+              data-mui-test="toolbar-am-btn"


Maybe avoid adding these props as long as we can avoid the need for them in tests? 🤔

LukasTy · 2023-03-02T15:45:30Z

packages/x-date-pickers/src/DateTimePicker/shared.tsx

@@ -74,6 +74,11 @@ export interface BaseDateTimePickerProps<TDate>
   * @default `utils.is12HourCycleInCurrentLocale()`
   */
  ampm?: boolean;
+  /**
+   * Display ampm controls under the clock (instead of in the toolbar).
+   * @default true


It seems that this is actually the PR resolving the need to remove this TODO.
Could you sync with next and make the necessary mentioned adjustment? 🤔

I forget it was also fixing it. Just added the related issue in the description

LukasTy

Leaving a few comments to finalize a few points. 😉

LukasTy · 2023-03-03T07:38:26Z

packages/x-date-pickers/src/DateTimePicker/shared.tsx

  /**
   * Display ampm controls under the clock (instead of in the toolbar).
-   * @default true
+   * @default true on desktop, false on mobile


Could you please apply the following diff to actually make this work as mentioned here? 🤔
Because currently, this place overrides defaults coming from Static, Desktop, and Mobile pickers. 🙈

diff --git a/packages/x-date-pickers/src/DateTimePicker/shared.tsx b/packages/x-date-pickers/src/DateTimePicker/shared.tsx index 8bc182198..68e0c1910 100644 --- a/packages/x-date-pickers/src/DateTimePicker/shared.tsx +++ b/packages/x-date-pickers/src/DateTimePicker/shared.tsx @@ -129,7 +129,6 @@ type UseDateTimePickerDefaultizedProps< | 'views' | 'openTo' | 'orientation' - | 'ampmInClock' | 'ampm' | keyof BaseDateValidationProps<TDate> | keyof BaseTimeValidationProps @@ -163,7 +162,6 @@ export function useDateTimePickerDefaultizedProps< }; }, [themeProps.localeText]); - const ampmInClock = themeProps.ampmInClock ?? true; const slots = themeProps.slots ?? uncapitalizeObjectKeys(themeProps.components); const slotProps = themeProps.slotProps ?? themeProps.componentsProps; return { @@ -177,7 +175,6 @@ export function useDateTimePickerDefaultizedProps< ampm, localeText, orientation: themeProps.orientation ?? 'portrait', - ampmInClock, // TODO: Remove from public API disableIgnoringDatePartForTimeValidation: themeProps.disableIgnoringDatePartForTimeValidation ?? @@ -205,7 +202,6 @@ export function useDateTimePickerDefaultizedProps< ...slotProps, toolbar: { ampm, - ampmInClock, ...slotProps?.toolbar, }, },

LukasTy · 2023-03-03T07:44:45Z

packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePicker.types.ts

@@ -26,6 +26,11 @@ export interface DesktopDateTimePickerSlotsComponentsProps<TDate>
 export interface DesktopDateTimePickerProps<TDate>
  extends BaseDateTimePickerProps<TDate>,
    DesktopOnlyPickerProps<TDate> {
+  /**
+   * Display ampm controls under the clock (instead of in the toolbar).
+   * @default true


Are you sure it's worth going with these overrides trying to provide a more correct default? 🤔
It's nice, I agree, but in that case, we'd need to fix an issue with DateTimePicker, maybe omit ampmInClock when creating DateTimePickerProps?
Or just go without overriding the default and show true on desktop, false on mobile 🙈
What's your take on it @flaviendelangle?

I did not realize it would also affect all the other component. I moved back to true on desktop, false on mobile for every one to avoid side effects

alexfauquette added component: TimePicker The React component. design: ux dx Related to developers' experience component: DateTimePicker The React component. labels Jun 8, 2022

alexfauquette requested a review from flaviendelangle June 9, 2022 08:48

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2022

DanailH changed the base branch from master to next September 12, 2022 13:08

alexfauquette mentioned this pull request Dec 21, 2022

[pickers] Use grid for modifying the layout #6900

Merged

flaviendelangle mentioned this pull request Dec 29, 2022

[docs] Cleanup the doc pages of the community pickers #7339

Merged

alexfauquette added 2 commits January 3, 2023 16:32

fix

59289d6

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

deb3974

alexfauquette force-pushed the fix-ampm-positions branch from 4d830fd to deb3974 Compare January 3, 2023 16:09

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 2023

alexfauquette commented Jan 4, 2023

View reviewed changes

alexfauquette added 3 commits January 4, 2023 15:54

fix arrow switch poosition

1f0802d

fix consistency in DateTime with clock ampm

462877e

fix date-time-toolbar ampm display in landscape mode

d9cc9a8

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 9, 2023

alexfauquette added 3 commits January 9, 2023 16:36

design refinement

188ab3d

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

cc1f884

scripts

ac37d01

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 9, 2023

scripts

c54dbf3

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

6919963

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 13, 2023

LukasTy mentioned this pull request Jan 16, 2023

[pickers] Remove WrapperVariantContext usage #7372

Closed

9 tasks

LukasTy approved these changes Jan 16, 2023

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023

alexfauquette added 2 commits January 17, 2023 10:38

small fixes

0d9913e

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

c3376bd

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023

fix

c466439

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

ebff384

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 17, 2023

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

eb11afb

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 14, 2023

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

9826473

alexfauquette requested a review from LukasTy March 1, 2023 14:03

LukasTy reviewed Mar 2, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/next' into fix-ampm-positions

c8b3d67

alexfauquette changed the title ~~[TimePicker] Fix ampm buttons position~~ [pickers] Fix am/pm buttons position and responsiveness Mar 2, 2023

Lukas feetbacks

4feae6b

LukasTy reviewed Mar 3, 2023

View reviewed changes

alexfauquette added 2 commits March 3, 2023 09:25

Lukas feedbacks

d776983

DateTimePickerPropremove TS overrides

1addb5f

alexfauquette merged commit 4a08283 into mui:next Mar 3, 2023

oliviertassinari added design This is about UI or UX design, please involve a designer and removed design: ux labels Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pickers] Fix am/pm buttons position and responsiveness #5149

[pickers] Fix am/pm buttons position and responsiveness #5149

alexfauquette commented Jun 8, 2022 •

edited by LukasTy

Loading

mui-bot commented Jun 8, 2022 •

edited

Loading

alexfauquette commented Jun 9, 2022

flaviendelangle commented Jul 21, 2022

alexfauquette commented Jul 25, 2022

github-actions bot commented Sep 1, 2022

alexfauquette Jan 4, 2023

github-actions bot commented Jan 9, 2023

alexfauquette commented Jan 9, 2023

LukasTy left a comment

LukasTy Jan 16, 2023 •

edited

Loading

alexfauquette Jan 17, 2023

LukasTy Jan 17, 2023

flaviendelangle Jan 18, 2023

alexfauquette Jan 18, 2023

github-actions bot commented Jan 17, 2023

github-actions bot commented Jan 17, 2023

github-actions bot commented Jan 17, 2023

LukasTy left a comment •

edited

Loading

LukasTy Mar 2, 2023

LukasTy Mar 2, 2023

alexfauquette Mar 2, 2023 •

edited

Loading

LukasTy left a comment

LukasTy Mar 3, 2023

LukasTy Mar 3, 2023 •

edited

Loading

alexfauquette Mar 3, 2023

[pickers] Fix am/pm buttons position and responsiveness #5149

[pickers] Fix am/pm buttons position and responsiveness #5149

Conversation

alexfauquette commented Jun 8, 2022 • edited by LukasTy Loading

Changelog

mui-bot commented Jun 8, 2022 • edited Loading

Netlify deploy preview

Updated pages

alexfauquette commented Jun 9, 2022

flaviendelangle commented Jul 21, 2022

alexfauquette commented Jul 25, 2022

github-actions bot commented Sep 1, 2022

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2023

alexfauquette commented Jan 9, 2023

LukasTy left a comment

Choose a reason for hiding this comment

LukasTy Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2023

github-actions bot commented Jan 17, 2023

github-actions bot commented Jan 17, 2023

LukasTy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

LukasTy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasTy Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette commented Jun 8, 2022 •

edited by LukasTy

Loading

mui-bot commented Jun 8, 2022 •

edited

Loading

LukasTy Jan 16, 2023 •

edited

Loading

LukasTy left a comment •

edited

Loading

alexfauquette Mar 2, 2023 •

edited

Loading

LukasTy Mar 3, 2023 •

edited

Loading