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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pickers] Refine referenceDate behavior in views #10863

Merged
merged 12 commits into from
Nov 9, 2023

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Oct 31, 2023

Part of #10747

Align the behavior of referenceDate on the views (DateCalendar, DigitalClock, and MultiSectionDigitalClock).
The behavior on TimeClock already makes sense.

  • Make the referenceDate day focusable (tabindex="0") if no value is present in DateCalendar instead of today's day
  • Make the referenceDate time focusable (tabindex="0") and scroll the options list to it if no value is present and the date maps to an available option in DigitalClock
  • Make the referenceDate time sections focusable (tabindex="0") and scroll the options lists to them if no value is present and the date maps to an available option in MultiSectionDigitalClock

WDYT about MonthCalendar and YearCalendar, should we align it to also not mark the reference month and day as selected, but only focus it? 馃

Update

I've also updated the behavior of YearCalendar and MonthCalendar.
P.S. The Argos diff is expected if we go with the proposal, because YearCalendar and MonthCalendar do not have the autoFocus prop set to true when used outside of DateCalendar.

Before

Screenshot 2023-11-06 at 17 54 17 Screenshot 2023-11-06 at 17 55 10

After

Screenshot 2023-11-06 at 17 54 47 Screenshot 2023-11-06 at 17 54 51

@LukasTy LukasTy added 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 labels Oct 31, 2023
@LukasTy LukasTy self-assigned this Oct 31, 2023
@mui-bot
Copy link

mui-bot commented Oct 31, 2023

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

Generated by 馃毇 dangerJS against 41f7aa4

@croraf
Copy link

croraf commented Nov 1, 2023

I checked how DateTimePicker behaves in the PR. Few comments:

  1. Currently the field's onChange is called even if OK button is not pressed. I can maybe understand that the onChange gets called on minutes selection when the popup closes (but even this is not good UX because there is an OK button, and the user might want to first set the minutes, then hours for example).
    Currently, even when hour or date gets clicked the onChange is called with the new value. Not good. The onChange should be called only when OK is clicked.

  2. I see now the intent of referenceDate. There are several variations on how someone would like it to behave. For the date part, one might want to:

  • just focus the reference month in the popup (current behavior)
  • highlight reference date in the popup (your implementation)
  • preselect reference date in the popup (I'd like to have this)
  1. Each of the three implementations have variations on how the time part can behave. The third bullet that I want would have date, hours and minutes preselected in the popup. That would allow the user to just click OK if they are satisfied with the proposed preselection. The value of the field would be null until OK is clicked.

@LukasTy
Copy link
Member Author

LukasTy commented Nov 2, 2023

@croraf Thank you for your feedback. 馃檹

  1. Currently the field's onChange is called even if OK button is not pressed. <...>

Your insight makes sense and we have separate issues to tackle the onChange behavior (it is a huge and very complex topic).
Nothing in the event handling behavior has been changed. That's a different topic. In this PR I tried solving the inconsistency problem we have observed between the DateCalendar and DigitalClock behaviors.

2. There are several variations on how someone would like it to behave.

My changes do not introduce any extra highlighting. It only changes the elements, which receive tabIndex="0" and thus are the first ones that are focused, when views change. 馃槈

preselect reference date in the popup (I'd like to have this)

As discussed in the issue, this is indeed a new feature that has to be opt-in and as far as I can see - it is the main topic of the original issue, correct me if I'm wrong? 馃

@croraf
Copy link

croraf commented Nov 2, 2023

Ok. I need to go push the change of the onChange behavior first, and then push for the new feature that I need.

@flaviendelangle
Copy link
Member

WDYT about MonthCalendar and YearCalendar, should we align it to also not mark the reference month and day as selected, but only focus it? 馃

It would probably make sense indeed

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.

Looks great ! 馃憣

@michelengelen michelengelen changed the base branch from master to next November 6, 2023 14:18
@LukasTy
Copy link
Member Author

LukasTy commented Nov 7, 2023

It would probably make sense indeed

I've made the necessary changes, updated the description with examples and requesting a re-review. 馃槈

Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Very nice improvement 馃帀 LGTM 馃挴

@LukasTy LukasTy merged commit c7682ff into mui:next Nov 9, 2023
18 checks passed
@LukasTy LukasTy deleted the update-reference-date-behavior branch November 9, 2023 15:10
LukasTy added a commit to LukasTy/mui-x that referenced this pull request Feb 19, 2024
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.

None yet

5 participants