Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

feat(video): updated to match new specs#2452

Merged
agliga merged 4 commits into15.2.0from
video-updates
Mar 26, 2025
Merged

feat(video): updated to match new specs#2452
agliga merged 4 commits into15.2.0from
video-updates

Conversation

@agliga
Copy link
Copy Markdown
Contributor

@agliga agliga commented Mar 6, 2025

⚠️ Note please read before reviewing

I included the CSS here currently only for review. BEFORE I merge, I will push this to skin and remove the CSS file. It is easier to review it here and change it and then update skin rather than creating two PRs and keeping them in sync. There's a lot of styles which are specific to the configuration.

Description

  • Created as a draft PR to get early reviews
  • Added current time and end time components (copied the shaka player components, but split them up into two). This allows us to the current time before the video progress and total time after
  • Added a custom mute/unmute control. This is mainly for the icon, but might be extended to have volume popover (currently on hold)
  • Shifted video time scrubber to be inline to controls. I looked at the best way to do it, and what shaka themes overrides did is to position this using margin and absolute positioning. I don't like this solution, but there doesn't seem to be a better way (I tried forking it but there were issues with updating the time, plus any changes we have to fix our forked code)
  • Removed overflow. Moved report to be inline.
  • Moved CC to be inline (note, CC only shows when a video has CC, none of our examples have it, so it won't show)
  • Misc cleanup of old code like removing some unused state variables

References

#2419

this.handleSuccess();
})
.catch((e: Error) => {
console.log(e);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Im considering to leave this here. Any errors are swallowed and is very hard to debug.
I can also remove if most people think its not necessary

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.

I think leaving it makes sense, especially if we're sure this error only shows up when the video actually doesn't load properly

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 6, 2025

🦋 Changeset detected

Latest commit: 2fb8321

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

This PR includes changesets to release 1 package
Name Type
@ebay/ebayui-core 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

@agliga agliga marked this pull request as ready for review March 6, 2025 22:10
Copy link
Copy Markdown
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Looks good! Just one nit, and then I'll approve after you move the .css file to skin

Copy link
Copy Markdown
Contributor

@saiponnada saiponnada left a comment

Choose a reason for hiding this comment

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

Few things I noticed,

As part of a simple keyboard test

  • Current time is being focused which is unnecessary and after looking further looks like it is a button. What does this button do?
  • I understand you had issues with positioning this seek bar. But the problem here is focus order is not aligning with visual order i.e, focus jumps from play button to timestamp and after full screen button it returns back to the track where a user has the ability to fast track or rewind

When I did a quick screen reader test, few things I noticed

  • When I am on seek slider, it announces some gibberish number. What would be ideal is on what time stamp we are on and what is overall duration of the video.
  • IMO it is always nice to CC button visible but disabled if no captions available

@agliga
Copy link
Copy Markdown
Contributor Author

agliga commented Mar 21, 2025

Few things I noticed,

As part of a simple keyboard test

  • Current time is being focused which is unnecessary and after looking further looks like it is a button. What does this button do?

I disabled the button so its no longer in focus state

  • I understand you had issues with positioning this seek bar. But the problem here is focus order is not aligning with visual order i.e, focus jumps from play button to timestamp and after full screen button it returns back to the track where a user has the ability to fast track or rewind

That is unfortunately the recommended way to do it in shaka player. I don't like the change but there's no other way to do this.

When I did a quick screen reader test, few things I noticed

  • When I am on seek slider, it announces some gibberish number. What would be ideal is on what time stamp we are on and what is overall duration of the video.

So again, this is what shaka player is providing. It could be due to the video we are testing.

  • IMO it is always nice to CC button visible but disabled if no captions available

Again, shaka player implementation.

I think the conclusion is to feel free to file a bug on their repo. They seem pretty responsive on that. We had an issue that they fixed pretty quickly after we filed it.

@agliga
Copy link
Copy Markdown
Contributor Author

agliga commented Mar 24, 2025

This should be ready for re-review, removed the CSS and pushed it to skin

@saiponnada saiponnada self-requested a review March 26, 2025 20:13
Copy link
Copy Markdown
Contributor

@saiponnada saiponnada left a comment

Choose a reason for hiding this comment

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

Looks like we have few limitations with third party library in making this accessible. We need to eventually figure out a solution that can solve these issues in long term. For now I am okay with moving forward.

@agliga agliga merged commit 74fcd1f into 15.2.0 Mar 26, 2025
5 checks passed
This was referenced Mar 25, 2025
@agliga agliga deleted the video-updates branch March 31, 2025 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants