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 PageUp and PageDown support for time components #14812

Merged

Conversation

arthurbalduini
Copy link
Member

@arthurbalduini arthurbalduini commented Oct 3, 2024

Closes #14548

To achieve it on the DigitalClock and MultiSectionDigitalClock I had to manipulate the list focus on our side. This approach was discussed with Diego and Jun.

@arthurbalduini arthurbalduini added accessibility a11y 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 3, 2024
@mui-bot
Copy link

mui-bot commented Oct 3, 2024

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

Generated by 🚫 dangerJS against 5355f67

@arthurbalduini arthurbalduini marked this pull request as ready for review October 4, 2024 09:45
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.

Great to see the accessibility improving here
Thanks for synchronizing with the core team 👍

packages/x-date-pickers/src/DigitalClock/DigitalClock.tsx Outdated Show resolved Hide resolved
@arthurbalduini arthurbalduini changed the title [pickers] Add PageUp and PageDown support for TimeClock component [pickers] Add PageUp and PageDown support for TimeClock, DigitalClock and MultiSectionDigitalClock components Oct 4, 2024
Copy link
Member

@LukasTy LukasTy 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, this is a nice improvement! 👍
There could be ideas on how to improve behavior with different timeSteps settings, but that's debatable and I'd say a question we should raise only when community demands it. 👍

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👍
Leaving a final nitpick for the tests in general.
But feel free to ignore it as we'd need to add eslint ignores to allow for this.
We can keep it for when we refactor to use userEvent. 👍

@LukasTy LukasTy changed the title [pickers] Add PageUp and PageDown support for TimeClock, DigitalClock and MultiSectionDigitalClock components [pickers] Add PageUp and PageDown support for time components Oct 10, 2024
@LukasTy LukasTy added the component: TimePicker The React component. label Oct 10, 2024
@arthurbalduini arthurbalduini merged commit 4bc5acc into mui:master Oct 10, 2024
18 checks passed
@arthurbalduini arthurbalduini mentioned this pull request Oct 10, 2024
Comment on lines +302 to +304
if (!listRef.current) {
return;
}
Copy link
Member

@oliviertassinari oliviertassinari Oct 11, 2024

Choose a reason for hiding this comment

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

This code is impossible to reach?

Suggested change
if (!listRef.current) {
return;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory listRef.current can be null, so this is meant to assure null safety on the lines below (and prevent TS complaints 😅)

Copy link
Member

@oliviertassinari oliviertassinari Oct 12, 2024

Choose a reason for hiding this comment

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

In theory listRef.current can be null

Only if the ref is not applied to the right element. This sounds unlikely to happen in a future refactoring, not with our test suite.

I think that using listRef.current! so it gets transpiled to listRef.current is much better. No extra bundle size or runtime time spent on this. If done at scale, I would hope this brings more than a +1% improvement.

Copy link
Member

Choose a reason for hiding this comment

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

That was my suggestion to go the safer route and prevent cases where DOM could be tampered with.
But given most other cases, where <x>Ref.current is accessed, we could indeed simplify to listRef.current!.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense ! I will add it to this PR
Thanks for the suggestion 🙏

Comment on lines +57 to +58
const children = listElement.children;
return Array.from(children).findIndex((child) => child === getActiveElement(document));
Copy link
Member

Choose a reason for hiding this comment

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

How about we simplify it down to?

Suggested change
const children = listElement.children;
return Array.from(children).findIndex((child) => child === getActiveElement(document));
return Array.from(listElement.children).indexOf(getActiveElement(document)!);

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed IndexOf is more performatic than findIndex
Opened this PR for it

Thanks ! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: pickers This is the name of the generic UI component, not the React module! component: TimePicker The React component. enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] TimeClock PageUp, PageDown keyboard shortcuts
5 participants