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.

Sign up for GitHub

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

Jump to bottom

fix: improve sd-video a11y #1644

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

fix: improve sd-video a11y #1644

wants to merge 14 commits into from

Conversation

paulovareiro29
Copy link
Contributor

@paulovareiro29 paulovareiro29 commented Nov 14, 2024 •
edited
Loading

Description:

Closes #1494, #1529

Note: The placeholder video has been replaced by the solid awards video, as suggested by @coraliefeil.

Definition of Reviewable:

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 14, 2024 •
edited
Loading

🚀 Storybook has been deployed for branch fix_improve-sd-video-a11y

smfonseca
smfonseca reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@smfonseca smfonseca left a comment

Choose a reason for hiding this comment

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

@paulovareiro29 please update the sd-video template because it's still referencing the old placeholder video

packages/docs/src/stories/components/video.stories.ts Outdated Show resolved Hide resolved
@smfonseca smfonseca removed their assignment Nov 14, 2024
@MartaPintoTeixeira
Copy link
Contributor

MartaPintoTeixeira commented Nov 14, 2024 •
edited
Loading

Changes requested:
instead of: "Video Element With Poster Slot" change to "Video Element with Poster Slot"
instead of: "Video With Description" change to "Video with Description"
instead of: "Video With Copyright" change to "Video with Copyright"

@MartaPintoTeixeira
Copy link
Contributor

Screenshot 2024-11-14 at 17 28 36

For the tmplt sample: "Video With Description" the playbutton seems to be broken if you play around with responsive.

@MartaPintoTeixeira
Copy link
Contributor

MartaPintoTeixeira commented Nov 14, 2024 •
edited
Loading

For our templates. We should change the poster image for the video. Can we re-use the one used in the video provided or should I provide one?

@paulovareiro29
Copy link
Contributor Author

@MartaPintoTeixeira
Regarding the play button not being vertically aligned on video with description, this is because the alignment is being calculated using the video description.
I have also noticed the video can be played by clicking the description (figcaption). By fixing one issue, it will fix both.
I believe this should be tackled on a separate ticket.

Regarding the poster, can you please provide one? Thanks

@paulovareiro29
Copy link
Contributor Author

paulovareiro29 commented Nov 15, 2024 •
edited
Loading

Vertical alignment issue and description click playing the video, will be addressed here #1650

EDIT: Will be fixed here #1631. #1650 is a duplicated issue

paulovareiro29 pushed a commit that referenced this pull request Nov 18, 2024
@semantic-release-bot
chore(release/placeholders): 2.0.0 [skip actions]
52f1b71 
# [@solid-design-system/placeholders-v2.0.0](placeholders/1.8.0...placeholders/2.0.0) (2024-11-18)

* chore!: replace ui-placeholder-video with sds-placeholder-video (#1657) ([77f8343](77f8343)), closes [#1657](#1657) [#1644](#1644)

### BREAKING CHANGES

* The `ui-placeholder-video.mp4` has been deleted and
replaced by `sds-placeholder-video.webm`.

This PR addresses replacing the current placeholder video with the SDS
@paulovareiro29 paulovareiro29 marked this pull request as ready for review November 18, 2024 14:34
@MartaPintoTeixeira
Copy link
Contributor

for the 1st sample please keep the "media" poster (as in figma)

@MartaPintoTeixeira
Copy link
Contributor

a ticket has been created to remove overlay option from sd-video #1665

@MartaPintoTeixeira
Copy link
Contributor

I will approve this PR leaving a note that are 2 related tickets to be fixed: #1650 + #1665

@mariohamann mariohamann removed their assignment Nov 20, 2024
@karlbaumhauer karlbaumhauer removed their assignment Nov 20, 2024
smfonseca
smfonseca reviewed Nov 21, 2024
View reviewed changes
Copy link
Contributor

@smfonseca smfonseca left a comment

Choose a reason for hiding this comment

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

@paulovareiro29 the poster is not announced, I'm not sure if it's because the focus in directly set in the play button. Can you please investigate this? Thank you

@smfonseca smfonseca removed their assignment Nov 21, 2024
@paulovareiro29
Copy link
Contributor Author

@paulovareiro29 the poster is not announced, I'm not sure if it's because the focus in directly set in the play button. Can you please investigate this? Thank you

@smfonseca since the poster is an image, it is only read-out-loud when the voiceover is reading the page content, not when it focus the play button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smfonseca smfonseca smfonseca approved these changes

@MartaPintoTeixeira MartaPintoTeixeira MartaPintoTeixeira left review comments

@karlbaumhauer karlbaumhauer Awaiting requested review from karlbaumhauer

@mariohamann mariohamann Awaiting requested review from mariohamann

Requested changes must be addressed to merge this pull request.

Assignees

@mariohamann mariohamann

@coraliefeil coraliefeil

@smfonseca smfonseca

@paulovareiro29 paulovareiro29

@Vahid1919 Vahid1919

@auroraVasconcelos auroraVasconcelos

@yoezlem yoezlem

@MartaPintoTeixeira MartaPintoTeixeira

Labels
None yet
Projects
Solid Design System Project Board
Status: 👀 In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat[dev]: ✨ implement A11y improvements to sd-video
9 participants
@paulovareiro29 @MartaPintoTeixeira @mariohamann @smfonseca @karlbaumhauer @coraliefeil @Vahid1919 @auroraVasconcelos @yoezlem