-
Notifications
You must be signed in to change notification settings - Fork 39
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
make disabled buttons interactable #1461
Conversation
00f33af
to
dbdb4a2
Compare
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.
Amazing work!
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.
Just a quick question, but other than that LGTM
@@ -25,3 +26,5 @@ afterEach(() => { | |||
.querySelectorAll('[data-tippy-root]') | |||
.forEach((tippy) => tippy.remove()); | |||
}); | |||
|
|||
global.TextEncoder = TextEncoder; |
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.
Is this change related to this 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.
yes, it's needed for the SSR test where I'm calling renderToString
Changes
Created new
<ButtonBase>
component used to replace all instances of<button>
. This component intercepts thedisabled
prop, converts it toaria-disabled
and manually disables clicks. This keeps the tooltip focusable and allows tooltips to appear. See https://css-tricks.com/making-disabled-buttons-more-inclusive/Other notes:
disabled
attribute is still used on first render to make it progressively enhanced.aria-disabled
is incorrect in the case of non-button elements (e.g. links), so an additionaldata-iui-disabled
attribute has been added. While "disabled links" don't make sense, this serves as a safeguard in case users decide to do such weird things.iui-button-base
class with a small reset for normalizing button styles. This caused some very minor image changes.Testing
Screen.Recording.2023-07-31.at.2.58.37.PM.mov
Screen.Recording.2023-08-01.at.2.15.19.PM.mov
Safari + VO:
Screen.Recording.2023-07-31.at.3.04.45.PM.mov
Firefox + NVDA:
Untitled.mov
^Note: These screen reader tests required focusing the button to read out the tooltip. Simply traversing to the button using the virtual cursor didn't announce the tooltip text; this might be a bug in Tooltip.
Docs
Added changesets.