Add title attribute to icons#8060
Conversation
Explain the meaning of the icon for screen readers (and mouse over). Hide "inactive" (low opacity) icons from screen readers. Remove opacity: 1 styling, it's the default opacity.
|
Sorry for the multiple force pushes / CI runs. Tried to get away with just using the GitHub interface to make a PR, but I just couldn't get the formatting right 😅 |
zanieb
left a comment
There was a problem hiding this comment.
Thank you! This is a great idea.
|
Thanks for the review @zanieb, will probably find some time to update the PR on monday. |
|
Changing the wording to "Fix available" / "Fix not available" might be confusing. The meaning of the latter can be ambiguous (as in, can be interpreted as "unfixable" instead of manually fixable). I do think announcing/clarifying that a rule can be fixed automatically is warranted as it's an exception to the default status quo of a linter. Similarly with the stable/unstable wording. I believe stating that a rule is stable is excessive information, only the special state of "preview" needs to be announced/clarified. I experimented with fully hiding (both visually and audibly) the "inactive" icons (while still maintaining the layout) and feel that makes the rules table appear less cluttered:
So that's what I'm now proposing with the update I just pushed. Note: Using I also tried adding a heading of "Notes" to the icon column, but felt like it wasn't adding that much, so I ended up removing it again. |
CodSpeed Performance ReportMerging #8060 will improve performances by 4.98%Comparing Summary
Benchmarks breakdown
|
| format!("<span style='opacity: 0.1'>{PREVIEW_SYMBOL}</span>") | ||
| format!("<span style='visibility: hidden'>{PREVIEW_SYMBOL}</span>") |
There was a problem hiding this comment.
Why include the symbol span at all if it's hidden? While I agree that hiding them is a cleaner experience, I don't think it's very clear what's going on when looking at a section of rules with no fixes / preview.
There was a problem hiding this comment.
It makes sure the icons always appear in the same spot (visibility hidden makes the element still take up space). I based this on the reasoning from @charliermarsh for the commit that added the faint opacity (c9d7c0d)
There was a problem hiding this comment.
Interesting, I actually like the variant in which we show the opaque icons -- I find it clearer, because it gives framing to the column themselves. It was inspired by ESLint's documentation: https://eslint.org/docs/latest/rules/.
There was a problem hiding this comment.
I agree with Charlie, it'd be preferable to keep the dimmed icons for now. I'd rather not bundle removing them with this accessibility related change.
There was a problem hiding this comment.
Thanks for the link to the eslint docs. I see they also use aria-hidden for the dimmed icons. I restored the opacity, set aria-hidden. Hope this is acceptable now :)
Summary
Explain the meaning of the icon for screen readers (and mouse over). Hide "inactive" (low opacity) icons from screen readers.
Remove opacity: 1 styling, it's the default opacity.
Without this change a screen reader will just read "Hammer and spanner test tube" for the last column in each row.