Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

fix: contributors list screen reader announcement #2889

Merged
merged 11 commits into from Oct 10, 2022

Conversation

EmmaDawsonDev
Copy link
Contributor

Description

The contributors list on the about page was announcing every person twice when read with a screen reader because link had a title and the image had an alt text. I removed the image alt text and left an empty string so the name is not read out twice. I also replaced title with an aria-label which has better support overall with different assistive technologies. The title attribute is not well supported unfortunately. Also added 'opens in new tab' to the label so that any blind users do not get disorientated by the change of context.

Updated snapshots

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run lint:js -- --fix and/or npm run lint:md -- --fix for my JavaScript and/or Markdown changes.
    • This is important as most of the cases your code changes might not be correctly linted
  • I have run npm run test to check if all tests are passing, and/or npm run test -- -u to update snapshots if I created and/or updated React Components.
  • I have checked that the build works locally and that npm run build work fine.
  • I've covered new added functionality with unit tests if necessary.

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2022

Codecov Report

Base: 66.02% // Head: 67.18% // Increases project coverage by +1.15% 🎉

Coverage data is based on head (c3969fc) compared to base (fd57b87).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2889      +/-   ##
==========================================
+ Coverage   66.02%   67.18%   +1.15%     
==========================================
  Files         118      130      +12     
  Lines        1351     1484     +133     
  Branches      342      364      +22     
==========================================
+ Hits          892      997     +105     
- Misses        422      447      +25     
- Partials       37       40       +3     
Impacted Files Coverage Δ
src/__fixtures__/hooks.tsx 100.00% <ø> (ø)
src/__fixtures__/page.tsx 94.11% <ø> (-5.89%) ⬇️
src/components/Alert/index.tsx 50.00% <ø> (ø)
src/components/AnimatedPlaceholder/index.tsx 100.00% <ø> (ø)
...components/ApiComponents/Components/ApiHeading.tsx 100.00% <ø> (+100.00%) ⬆️
...rc/components/ApiComponents/Components/ApiLink.tsx 100.00% <ø> (+100.00%) ⬆️
...ents/ApiComponents/Components/SourceLink/index.tsx 50.00% <ø> (+50.00%) ⬆️
...ApiComponents/Components/VersionSelector/index.tsx 100.00% <ø> (+100.00%) ⬆️
src/components/ApiComponents/index.tsx 100.00% <ø> (+100.00%) ⬆️
src/components/Article/index.tsx 100.00% <ø> (ø)
... and 103 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@EmmaDawsonDev
Copy link
Contributor Author

I've updated it to use the useIntl hook and checked that it still works with a screen reader (NVDA on windows). If there's any names (or anything else) that you want me to change let me know, I'm not well versed enough in the project to know if you have any naming conventions you follow.

src/i18n/locales/es.json Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

I left two tiny comments, but regardless, super work! I hope you didn't face many issues in achieving the final result.

Feedback is welcome if you feel any guidelines were missing or if anything could be improved. Thank you, again, for your contribution 💖

@EmmaDawsonDev
Copy link
Contributor Author

Thanks, I’ll fix it tomorrow 👍🏻 I did try without definedMessages but got myself in a mess with curly braces. Hopefully I can work it out once I’ve slept (and I now have your example above 😊 )

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

:shipit:

@ovflowd ovflowd merged commit cfda51d into nodejs:main Oct 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants