-
Notifications
You must be signed in to change notification settings - Fork 375
fix(Tabs): fixed keyboard use and positioning of TabsAndTable dropdowns #9093
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
fix(Tabs): fixed keyboard use and positioning of TabsAndTable dropdowns #9093
Conversation
|
Preview: https://patternfly-react-pr-9093.surge.sh A11y report: https://patternfly-react-pr-9093-a11y.surge.sh |
|
|
||
| const firstActionRef = React.useRef<HTMLButtonElement>(null); | ||
|
|
||
| const handleClick = (event: React.MouseEvent, ActionsToggleProps: CustomActionsToggleProps) => { |
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.
Could you add some comments here as to the purposes of these handlers? I don't think it will be apparent to consumers.
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 not sure if I know quite what you mean by not apparent? Is the potential confusion just coming from the names that are a bit too generic?
I could definitely say that the KeyDown handler is to enable keyboard navigation of the toggle, and change the names so that it's more clear what these handlers are for, but I'm not sure what there really is to explain about the Click handler.
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 guess I mean - why are they needed?
Users don't ususally need to wire up keydown and click handling for our basic dropdowns, so this won't be what they expect.
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 added a couple of comments and made the handler names more descriptive, WDYT?
nicolethoen
left a comment
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 also noticing that using keyboard to click the link, or select an item from the dropdown, it toggles the drawer on the side.
I think the keyboard handling for these table rows should be attached to the checkbox, not the whole row (or other elements on the row) so it may be a little different than a click event (although I am seeing the same issue with the click events on the links, too).
This behavior could be a follow up issue if it is more of an implementation detail on the demo itself and not indicative of further breaking changes needed on any of the components it uses.
|
@nicolethoen it does seem like the problems are just demo based rather than component based to me, I made #9100 to address the link selection issue. |
e1e2a97 to
f545013
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #9017
Additional issues:
Convenience link: https://patternfly-react-pr-9093.surge.sh/components/tabs/react-demos/tables-and-tabs/