-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(Tag): Add keyboard accessibility #7060
Conversation
Generate changelog in
|
fix(Tag): Add keyboard accessibilityBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Add generated changelog entriesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
ref: React.Ref<E>; | ||
} | ||
|
||
export function useInteractiveAttributes<E extends HTMLElement>( |
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.
nit: can we add tests for this hook with @testing-library/react-hooks
in a follow-up PR?
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.
Yeah, sgtm
onClick: interactive ? onClick : undefined, | ||
onFocus: interactive ? onFocus : undefined, | ||
onKeyDown: handleKeyDown, | ||
onKeyUp: handleKeyUp, |
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.
Would this function the same if we were to conditionally render the inclusion of all the handlers based on the interactive
flag? e.g.
const handlers = {
onBlur: handleBlur,
onClick,
onFocus,
onKeyDown: handleKeyDown,
onKeyUp: handleKeyUp,
};
return [
resolvedActive,
{
...(interactive ? handlers : {}),
ref: mergeRefs(elementRef, ref),
tabIndex: interactive ? tabIndex : -1,
},
];
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.
Very likely I think, but I wanted to quite safe to avoid any unintentional side effects so I wanted to match the button behavior as close as possible.
CRBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes #4782
Changes proposed in this pull request:
In this PR we pull out the common accessibility functionality from the button components to be shared with the tag component. This enables the tag component to be keyboard accessible when
interactive={true}