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: sd-teaser and sd-teaser-media a11y #1625

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

Conversation

smfonseca
Copy link
Contributor

@smfonseca smfonseca commented Nov 7, 2024

Description:

This PR closes 1491, 1526 and 1527

Note: I removed the alt text from the images in the templates, because they are decorative images and do not need alt text since no important information is conveyed by them.

Definition of Reviewable:

  • Documentation is created/updated
  • Migration Guide is created/updated
  • E2E tests (features, a11y, bug fixes) are created/updated
  • Stories (features, a11y) are created/updated
  • relevant tickets are linked

Copy link

github-actions bot commented Nov 7, 2024

🚀 Storybook has been deployed for branch fix_sd-teaser-and-teaser-media-a11y

@MartaPintoTeixeira
Copy link
Contributor

on the variants the order is wrong. "primary" should be the last on the list to match the samples. can you change this please?

@MartaPintoTeixeira
Copy link
Contributor

change the "Meta Slot" description to:
Use the “meta” slot to add additional content to the teaser. 

paulovareiro29
paulovareiro29 previously approved these changes Nov 19, 2024
Copy link
Contributor

@paulovareiro29 paulovareiro29 left a comment

Choose a reason for hiding this comment

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

LGTM

@MartaPintoTeixeira
Copy link
Contributor

on teaser temlate instead of: "Unclickable Teaser With Button" change to "Unclickable Teaser with Button" please

@MartaPintoTeixeira
Copy link
Contributor

for teaser media template instead of "Teaser Media With Link" change to "Teaser Media with Link"

@MartaPintoTeixeira
Copy link
Contributor

for the Teaser Media Overrides sample template can you make it more similar to figma?
"Gender" copy size is different in storybook

@MartaPintoTeixeira
Copy link
Contributor

on the 1st sample for teaser in figma we have the vertical variant.

src="./placeholders/images/coins.jpg"
alt="Close-up of hands stacking coins into small piles on a table, suggesting financial planning or saving."
/>
<img class="w-full aspect-ratio" src="./placeholders/images/coins.jpg" alt="" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, if these really count as decorative. Would love to hear Debbies thoughts on that: https://solid-design-system.github.io/solid/fix_sd-teaser-and-teaser-media-a11y/?path=/docs/templates-teaser--docs

Choose a reason for hiding this comment

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

As in the examples the images are not decorative but delivering emotions (e.g. Future is an attitude -> man with young children) I would actually add a meaningful alt attribute.
I suggest to add a small a11y hint though which says something like in case your used image is decorative keep it empty...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I don't feel the images add relevant information to the text and are more "eye candy", but I guess this goes into what we discussed about acessibility not being an exact science. I will add back some alt texts and the suggested a11y hint.

@mariohamann mariohamann removed their assignment Nov 20, 2024
@karlbaumhauer karlbaumhauer removed their assignment Nov 20, 2024
@smfonseca
Copy link
Contributor Author

@MartaPintoTeixeira

on teaser temlate instead of: "Unclickable Teaser With Button" change to "Unclickable Teaser with Button" please

Updated

for teaser media template instead of "Teaser Media With Link" change to "Teaser Media with Link"

Updated

on the 1st sample for teaser in figma we have the vertical variant.

Updated

for the Teaser Media Overrides sample template can you make it more similar to figma?
"Gender" copy size is different in storybook

Synced directly on teams call. There's a ticket to make the headline font size overridable. Decision now was to update Figma to resemble Storybook.

@paulovareiro29 paulovareiro29 removed their assignment Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

feat[dev]: ✨ implement A11y improvements to sd-teaser and sd-teaser-media
10 participants