-
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): Mark tag as interactive when onClick is set #7073
Conversation
Generate changelog in
|
@@ -99,8 +99,9 @@ export const Tag: React.FC<TagProps> = React.forwardRef((props, ref) => { | |||
} = props; | |||
|
|||
const isRemovable = Utils.isFunction(onRemove); | |||
const isInterative = interactive || htmlProps.onClick != null; |
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.
It might be useful to allow someone to still explicitly set interactive: false
even when they provide an onClick
handler, in case that's behavior they're already relying on for some reason.
To do that, should we change this to interactive ?? htmlProps.onClick != null
and remove the default = false
on line 89 above?
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 good call
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.
should this interactive ?? htmlProps.onClick != null
defaulting behavior happen in useInteractiveAttributes
? right now I think every consumer of this hook would need to do something similar
and maybe it should be a disabled
option instead? if a component is using this hook I think we should infer it's interactive based on being provided an onClick
, then disabled
can be offered as a feature so a consumer doesn't have to do something like conditionally provide onClick
it seems like we ended up with interactive
as an option to this hook since the Tag
originally had it, but now we're walking back on interactive
needing to be set to get the interactive attributes
FixesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
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.
Can you update the comment about the default value in the docs as well? Otherwise LGTM.
@@ -57,6 +57,20 @@ describe("<Tag>", () => { | |||
assert.isTrue(handleRemove.calledOnce); | |||
}); | |||
|
|||
it("should be interactive when onClick is provided", () => { |
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.
Thanks for adding tests!
Invalidated by push of fe6ea5a
update docsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Ensures that when onClick is passed, the interactive prop is set so we get the best possible accessibility story.