Skip to content
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

Add title attribute to icons #8060

Merged
merged 3 commits into from
Oct 26, 2023
Merged

Add title attribute to icons #8060

merged 3 commits into from
Oct 26, 2023

Conversation

jaap3
Copy link
Contributor

@jaap3 jaap3 commented Oct 19, 2023

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.

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.
@jaap3
Copy link
Contributor Author

jaap3 commented Oct 19, 2023

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 😅

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This is a great idea.

@charliermarsh charliermarsh added the documentation Improvements or additions to documentation label Oct 19, 2023
@jaap3
Copy link
Contributor Author

jaap3 commented Oct 20, 2023

Thanks for the review @zanieb, will probably find some time to update the PR on monday.

@jaap3
Copy link
Contributor Author

jaap3 commented Oct 23, 2023

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:

Screenshot 2023-10-23 at 10 32 16

So that's what I'm now proposing with the update I just pushed.

Note: Using visibility: hidden also removes the node from the accessibility tree, so it's not announced https://developer.mozilla.org/en-US/docs/Web/CSS/visibility#accessibility_concerns

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.

@jaap3 jaap3 requested a review from zanieb October 23, 2023 08:33
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 23, 2023

CodSpeed Performance Report

Merging #8060 will improve performances by 4.98%

Comparing jaap3:patch-1 (5b98249) with main (67b0434)

Summary

⚡ 4 improvements
✅ 21 untouched benchmarks

Benchmarks breakdown

Benchmark main jaap3:patch-1 Change
linter/all-rules[large/dataset.py] 172.5 ms 164.4 ms +4.9%
lexer[unicode/pypinyin.py] 583.1 µs 559.1 µs +4.29%
lexer[pydantic/types.py] 4 ms 3.8 ms +4.62%
lexer[large/dataset.py] 8.9 ms 8.5 ms +4.98%

Comment on lines 32 to 34
format!("<span style='opacity: 0.1'>{PREVIEW_SYMBOL}</span>")
format!("<span style='visibility: hidden'>{PREVIEW_SYMBOL}</span>")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@charliermarsh charliermarsh merged commit 25d4dda into astral-sh:main Oct 26, 2023
17 checks passed
@jaap3 jaap3 deleted the patch-1 branch October 26, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants