Skip to content

Conversation

@ChristopherChudzicki
Copy link
Contributor

What are the relevant tickets?

For https://github.com/mitodl/hq/issues/5708

Description (What does it do?)

Puts the search count inside the <label> for screenreaders

Screenshots (if appropriate):

No visual change

Screenshot 2024-10-22 at 4 57 11 PM Screenshot 2024-10-22 at 4 59 14 PM

How can this be tested?

  1. Check that the search page has no visual change
    • OK I guess there is one... the mouse cursor is pointer on the count now, too.
    • you might want to check that the checkboxes change color the same as prod on hover, etc
  2. Check that the checkbox has count as part of its label in the accessibility tree. You could also enable a screenreader like voiceover, if you want. (See screenshot above)

Checklist:

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review October 22, 2024 21:00
@ChristopherChudzicki ChristopherChudzicki changed the title Cc/count in label Search count inside label Oct 22, 2024
@jonkafton jonkafton self-assigned this Oct 23, 2024
@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Oct 23, 2024
Copy link
Contributor

@jonkafton jonkafton left a comment

Choose a reason for hiding this comment

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

Just the one mistype on the selector.

For my curiosity, what is the intent of this change? I would have thought the count to be semi-auxiliary information that clutters the audio. I guess all information should be available to screen readers, though doesn't labels such as "With Certificate 20" break the semantics? Would role="presentation" help, or does that default the point?

.facet-visible .facet-label label:hover,
.facet-visible input:hover + .facet-label label {
color: ${({ theme }) => theme.custom.colors.darkGray2};
&.chcked,
Copy link
Contributor

Choose a reason for hiding this comment

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

&.checked here (impact is that we don't get the darker text when checked).

@jonkafton jonkafton added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Oct 23, 2024
@jonkafton jonkafton removed their assignment Oct 23, 2024
@ChristopherChudzicki
Copy link
Contributor Author

I would have thought the count to be semi-auxiliary information that clutters the audio. I guess all information should be available to screen readers, though doesn't labels such as "With Certificate 20" break the semantics?

I can see both viewpoints (clutter audio / useful info) but am deferring to the screenreader review on this one. This particular piece of screenreader feedback—and many others we're working on lately—come from a full-time screenreader user + accessibility expert who works for MIT.

@ChristopherChudzicki ChristopherChudzicki merged commit 47ee91e into main Oct 23, 2024
12 checks passed
@rhysyngsun rhysyngsun deleted the cc/count-in-label branch February 7, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants