-
Notifications
You must be signed in to change notification settings - Fork 215
Fixes and visual updates for help and info icons + install KDS 5.2.2 #5213
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
base: unstable
Are you sure you want to change the base?
Conversation
@tomiwaoLE I think this should be aligned with your design and thoughts, but in the course of work, I've discovered few more places that will be affected by changes to info "i" tooltip. Before I open this for code review, would you please check all the screenshots above and confirmed it'd be good to go? |
This all works @MisRob. Thanks for the hard work of capturing all these use cases |
- Fixes click on the help “?” icon button not opening the help dialog - Updates the “i” icon to match “?” icon to achieve consistent experience - Installs latest KDS version with related updates
9d7e81d
to
e54dfe9
Compare
The only change I did in the force push was removal of temporary KDS fork installation in favor of installing now released 5.2.2 that has |
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.
The previous implementation was keyboard navigable. Although, that may not have been entirely appropriate and correct with regards to accessibility.
It seems to me the help tooltips shouldn't be keyboard navigable, but they should have association with the field. The documentation on aria-describedby
seems to fit well with this:
For example, a form control can have a description that is hidden by default and revealed on request using a disclosure widget like a "more information" icon. The sighted users click on the icon to view the description, while assistive technology users can immediately access it as the description is referenced from that form control with
aria-describedby
While the previous implementation wasn't correct, adding aria-describedby
seems like the appropriate replacement behavior now that keyboard navigation is gone, to maintain some semblance of accessibility :)
Thanks @bjester.
Definitely, I saw that pattern and I wouldn't mind improving some parts in this PR if you can think of anything - just not aware how to achieve better a11y without the prerequisites mentioned above. |
Ah okay @bjester, I think I get better how you meant it, thank you. There's more things. I think the first two points are relevant particularly to this PR. (3) and (4) is continuing discussion for future tooltips - if you had any further thoughts on that too, let me know. I'd be also happy to have you join PoC review as soon as I have it ready.
|
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 think this is a huge improvement, on many levels! Thanks for working with me @MisRob, to better Studio's accessibility
Summary
KIconButton
as is would cause visual regressions because it doesn’t yet account for smaller form icon use-cases. There was no clarity yet in what behaviors we want to make part of KDS, so for now we decided to start exploring better experience with Studio-specific icon button. The icon is 16px and contains further improvements such larger touch target area, and hover transition.KIcon
change needed for this PRTouch target area agreed with @radinamatic, and visual experience with @tomiwaoLE.
Screenshots
This is how all updated "i" and "?" icons in the screenshots below behave:
icons-recording.webm
Affected places
Reviewer guidance
Usual manual preview of few places mention above, and code review should do. You need to run
pnpm install
before running the development server.