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

fix: Verplaats het aria-label op de link naar de SVG en voeg de ontbrekende aria-current toe #423

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

Conversation

rianrietveld
Copy link
Contributor

@rianrietveld rianrietveld commented Jan 9, 2025

Best practice is om de toegankelijke naam van een link te beginnen met de visuele tekst. De dot in de tekst is overbodig, gebruik voor een korte pauze beter een komma.

De aria-current ontbrak op de link voor het verhaal “Link rondom image, current page” en is nu toegevoegd.

De toegankelijke naam staat op de SVG.
Vergelijkbaar met het geval van een img met een alt attribuut.
ZIe voor uitleg: https://www.htmhell.dev/adventcalendar/2024/1/

Preview: https://candidate-storybook-test-git-fix-logo-i-ae48c3-nl-design-system.vercel.app/?path=/docs/componenten-link--documentatie
Gerelateerd issue: #210

Best practive is to start the accessible name of a link with the visual text. The dot in het text is redundant, better use a comma for a short pauze.

The aria-current was missing on the link for the story “Link rondom image, current page”.
Copy link

vercel bot commented Jan 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
candidate ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 10:14am
candidate-storybook-test ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 10:14am
evil-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 10:14am

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c40b215) to head (c986084).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #423   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines          391       391           
  Branches        30        32    +2     
=========================================
  Hits           391       391           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rianrietveld rianrietveld changed the title fix: change aria-label on link and add missing aria-current fix: Verplaats het aria-label op de link naar de SVG en voeg de ontbrekende aria-current toe Jan 9, 2025
Copy link
Member

@matijs matijs left a comment

Choose a reason for hiding this comment

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

We moeten er op een later tijdstip en zeker in een aparte PR nog voor kiezen om het SVG image ergens anders onder te brengen:

  • in het storybook shared package als het ook in andere storybooks gebruikt moet kunnen worden
  • in packages/storybook-test/src zodat we de Link stories er verder niet mee lastig vallen

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.

3 participants