Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(embed): make component focusable, only show control/placeholder on video #1758

Merged
merged 30 commits into from
Aug 13, 2019

Conversation

lucivpav
Copy link
Contributor

@lucivpav lucivpav commented Aug 5, 2019

This PR resolves issue #1767, which states that:

the pause control button is visible, even if we are using an iframe in Embed component.

This PR also resolves issue #1742 by making the Embed component focusable using keyboard.
image

Other changes

  • control button, placeholder is not shown when video prop not set
  • background set to colorScheme.brand.background
  • added YouTube example

TODO

  • accessibility is broken even on master, this will be fixed in another PR

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #1758 into master will increase coverage by 0.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1758      +/-   ##
==========================================
+ Coverage   69.77%   69.78%   +0.01%     
==========================================
  Files         870      870              
  Lines        7486     7496      +10     
  Branches     2176     2204      +28     
==========================================
+ Hits         5223     5231       +8     
- Misses       2255     2257       +2     
  Partials        8        8
Impacted Files Coverage Δ
...t/src/themes/teams/components/Embed/embedStyles.ts 0% <0%> (ø) ⬆️
packages/react/src/components/Embed/Embed.tsx 96.15% <100%> (+1.7%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f594298...77af826. Read the comment docs.

@vercel
Copy link

vercel bot commented Aug 5, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://stardust-react-git-fix-embed-focus.stardust-ui.now.sh

@lucivpav
Copy link
Contributor Author

lucivpav commented Aug 9, 2019

@alinais please review this PR again, it has changed quite a bit since last time

@vercel vercel bot temporarily deployed to staging August 13, 2019 13:22 Inactive
Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

👍

CHANGELOG.md Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants