-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(Truncate, Progress): enable truncated tooltip via keyboard #11731
base: main
Are you sure you want to change the base?
Conversation
Preview: https://patternfly-react-pr-11731.surge.sh A11y report: https://patternfly-react-pr-11731-a11y.surge.sh |
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.
Looks good. Unless you want to take a stab at it testing can be handled as part of the unit test revamp epic. Only nit is the handler naming being onFocus
for both mouse enter and focus, but something like onFocusAndMouseEnter
may be bit much.
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.
Forgot the original issue is for Truncate and Progress. Can you add similar logic for the Truncate component, or we can open a followup for that since we should add tests for that more immediately
Ah I misread the original issue, will update to add Truncate |
Instead of |
At least you weren't the one to write the issue and also misread 😆 And 👍🏼 to the rename |
Updated. As far as testing, what do you think is the best approach for testing this out? The Truncate tests are mocked out such that the tooltip is always rendered so I'm a little confused on how to check that the tooltip isn't visible by default or with large parent widths / changes state on resize or trigger |
We could probably get away with trying to test for 1) tabindex not ebing set by default and 2) tabindex being set when truncate is intended to occur. Otherwise an integration test might be the next best way and just checking the tooltip is triggered and the content can be focused |
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.
For real this time
What: Closes #11384
Progress:
tabIndex
to allow keyboard focus and anupdateTooltip
handler to also trigger the tooltip creation. TheuseEffect
block is there to refocus the title text after the state updates and rerenders with the tooltip.Truncate:
tabIndex
to the subparent to allow keyboard focus