-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[charts] Make axis highlight reflect the keyboard interaction #19631
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
Conversation
Deploy preview: https://deploy-preview-19631--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Instrumentation Performance ReportMerging #19631 will not alter performanceComparing Summary
Footnotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes broke regular axis highlighting. Also, should the "keyboard" highlight only trigger when the highlight options are enabled?
Thanks for the heads up. It's probably just because I returned an empty array instead of |
Should it respect the last changed value? It feels odd that the keyboard always has preference. Screen.Recording.2025-09-20.at.17.30.13.mov |
An other approach could be to update the axis highlight during keyboard interaction. Something like calling |
I think it might work better than current 🤔 |
It was trickier than expected. The axis highlight only store the pointer coordinate. We did that such that the axis-highlight sty in sync with any axis update. The new approach store in the state a value We then have selectors for all options (controlled, pointer, keyboard) and a final selector will pick the correct one |
...rc/internals/plugins/featurePlugins/useChartKeyboardNavigation/useChartKeyboardNavigation.ts
Outdated
Show resolved
Hide resolved
return undefined; | ||
} | ||
|
||
let axisId: AxisId | false | undefined = 'xAxisId' in seriesConfig && seriesConfig.xAxisId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we always want to use the x axis here, even if we're running the selectorChartsKeyboardYAxisIndex
selector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭
…tKeyboardNavigation/useChartKeyboardNavigation.ts Co-authored-by: Bernardo Belchior <[email protected]> Signed-off-by: Alexandre Fauquette <[email protected]>
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if users will want to have the last hover highlight to be the start of the keyboard highlight.
Should be good for now though
Btw the accessibility page is not on the sidebar, was that intentional? 🤔 |
Yes, I'm planning to add it when keyboard navigation is done. Not an issue if user find the page, but I prefer to wait a bit before advertising it |
Follow up on keyboard navigation
The idea is to use the keyboard interaction state for highlight if the state is not controlled.
By priority, we show:
https://deploy-preview-19631--material-ui-x.netlify.app/x/react-charts/accessibility/#keyboard-support
Capture.video.du.2025-09-19.15-36-50.mp4
To do in a follow up: update the same way the item highlight