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

Project a11y / Accessibility #56

Open
wants to merge 58 commits into
base: main
Choose a base branch
from

Conversation

joheri1
Copy link

@joheri1 joheri1 commented Oct 6, 2024

JoyceKuode and others added 28 commits September 20, 2024 12:36
ction to the JavaScript file. Remove comments not in use
… main content at the top of the site to improve navigation.
…rtSelect, to improve accessibility for screen reader users
…clickable areas to meet the recommended touch target size.
Add triple backtick\ns at the end of one code snipets.
n the main section for the album container, and add aria-live polite to make the screen rea
der read the text on each album card. Update the readme file with If I had more time.
…e main section for the album container, and add aria-live = polite, so that screen reader read the text in the album cards
Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Thanks for sharing your process in your readme, you’ve done an extensive work this week!

Nice job with aria labels, skip links, alt tags and usage of semantic elements (including label for form elements) 👍

Your lighthouse report comes back good and you have all sufficient contrasts. Keep up the good work!

Copy link

@HeleneWestrin HeleneWestrin left a comment

Choose a reason for hiding this comment

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

Really nice accessibility improvements, Johanna! ⭐

Some suggestions to improve it even further:

  1. The focus styling is quite hard to see. Consider changing the focus styling to be more prominent. One idea could be to add a general :focus-visible to all focusable elements, like this for example:
    *:focus-visible { outline: 2px solid white; outline-offset: 2px; }

  2. Adding the number of albums before the album list could be helpful for screen reader users. Then that text could also be updated after the user has filtered so they know how many matches their filter had.

  3. WCAG recommends that no text should be under 12px in size. I noticed the artist name on mobile is only 0.65em (= 10.4px). Might be a good idea to change to at least 0.75em (= 12px).

  4. To avoid the filter and sort panels to be tabable even when they are not open you could trigger a class, you could call it .is-visible. On your panel classes you can add visibility: hidden; as default. Then when the user clicks the filter or sort button you could add the is-visible class which will have the CSS visibility: visible;

Again - great work with the accessibility 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants