Skip to content

Add heading_id option to card template#1577

Merged
tylersticka merged 3 commits into
v-nextfrom
feature/card-labelledby
Nov 15, 2021
Merged

Add heading_id option to card template#1577
tylersticka merged 3 commits into
v-nextfrom
feature/card-labelledby

Conversation

@tylersticka

Copy link
Copy Markdown
Member

Overview

@Paul-Hebert made a good suggestion to add support for the aria-labelledby property on articles to improve the experience of navigating between them via the VoiceOver rotor. After some research in the web-a11y Slack, I couldn't see any downside to adding this as an option.

I originally implemented the heading_id and a separate aria_labelledby property, but this felt a bit silly since they should always be the same. Instead, I simplified it down to a single heading_id option. If the heading_id is specified and the heading block isn't empty, it's automatically output to aria-labelledby on the root element as well. I wasn't sure if the label had to be on the heading or the immediate container of the heading text (in which case the link would need it, if present), but thankfully after some testing it appears the heading is always the most appropriate element.

Along the way, I found myself struggling a bit to update all of the static code samples, so I also replaced those with a templateSource function that builds the examples automatically. This should make the documentation for cards easier to maintain going forward.

Screenshots

Screen Shot 2021-11-09 at 1 19 23 PM

Testing

  1. On the deploy preview, try some of the Card examples in the VoiceOver rotor's "Articles" listing. You should see the article heading instead of a generic "Article."
  2. Review the docs page to verify that the source examples do not contain any unexpected regressions compared to what's live.

@changeset-bot

changeset-bot Bot commented Nov 9, 2021

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 07a546b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudfour/patterns Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify

netlify Bot commented Nov 9, 2021

Copy link
Copy Markdown

✔️ Deploy Preview for cloudfour-patterns ready!

🔨 Explore the source changes: 07a546b

🔍 Inspect the deploy log: https://app.netlify.com/sites/cloudfour-patterns/deploys/6192b2395fb509000864e355

😎 Browse the preview: https://deploy-preview-1577--cloudfour-patterns.netlify.app

@tylersticka
tylersticka marked this pull request as ready for review November 9, 2021 21:28
@tylersticka
tylersticka requested a review from a team November 9, 2021 21:28
@gerardo-rodriguez

Copy link
Copy Markdown
Member

2. Review the docs page to verify that the source examples do not contain any unexpected regressions compared to what's live.

@tylersticka Can you help me understand what you are referring to when you say "live"? Thank you! 🙂

@tylersticka

Copy link
Copy Markdown
Member Author

Can you help me understand what you are referring to when you say "live"? Thank you! 🙂

What's already been merged: https://v-next--cloudfour-patterns.netlify.app/

spaceninja
spaceninja previously approved these changes Nov 15, 2021

@spaceninja spaceninja left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code looks reasonable, and I can confirm that in the VO Rotor under "Articles" the generic "article" has been replaced with the title of the article. I defer to @gerardo-rodriguez for full approval on the a11y improvement, but this PR does what it says on the tin.

Comment thread src/components/card/card.stories.mdx Outdated
Comment thread src/components/card/card.stories.mdx Outdated
Co-authored-by: Gerardo Rodriguez <gerardo@cloudfour.com>

@gerardo-rodriguez gerardo-rodriguez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work, @tylersticka! 🎉

@tylersticka
tylersticka merged commit 1561c59 into v-next Nov 15, 2021
@tylersticka
tylersticka deleted the feature/card-labelledby branch November 15, 2021 19:46
@github-actions github-actions Bot mentioned this pull request Nov 15, 2021
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.

Cards should support aria-labelledby

3 participants