Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Remove scrolling="no" from Media Player #2280

Merged
merged 20 commits into from
Oct 2, 2019

Conversation

pjlee11
Copy link
Contributor

@pjlee11 pjlee11 commented Oct 1, 2019

Part of #2278

Overall change: Removes scrolling="no" attribute.


  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

Test this by regression checking npm run storybook.

@pjlee11 pjlee11 added ws-articles Tasks for the WS Articles Team a11y Accessibility-related task articles-av-epic ws-media The World Service media stream ws-media- LiveRadioV1 labels Oct 1, 2019
@pjlee11 pjlee11 self-assigned this Oct 1, 2019
@pjlee11 pjlee11 mentioned this pull request Oct 1, 2019
3 tasks
@pjlee11 pjlee11 changed the title Remove scrolling no for a11y reasons Add Title and remove scrolling="no" for a11y reasons Oct 1, 2019
@pjlee11 pjlee11 changed the title Add Title and remove scrolling="no" for a11y reasons Remove scrolling="no" for a11y reasons Oct 1, 2019
@@ -15,7 +15,7 @@ storiesOf('Components|Media Player', module)
.add('AMP', () => (
<AmpMediaPlayer
isAmp
src="https://www.test.bbc.co.uk/ws/av-embeds/articles/c3wmq4d1y3wo/p01k6msp"
src="https://www.test.bbc.co.uk/ws/av-embeds/articles/c3wmq4d1y3wo/p01k6msp/amp"
Copy link
Contributor Author

@pjlee11 pjlee11 Oct 1, 2019

Choose a reason for hiding this comment

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

Bug, might as well fix here

Copy link
Contributor

@simonsinclair simonsinclair left a comment

Choose a reason for hiding this comment

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

The changes look good, but I discovered a few things that might need addressing.

@12 12 changed the title Remove scrolling="no" for a11y reasons Remove scrolling="no" and add required title prop for a11y reasons Oct 2, 2019
@pjlee11 pjlee11 changed the title Remove scrolling="no" and add required title prop for a11y reasons Remove scrolling="no" from Media Player Oct 2, 2019
Copy link
Contributor

@ibMadbouly ibMadbouly left a comment

Choose a reason for hiding this comment

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

👍 LGTM 👍

@pjlee11 pjlee11 merged commit 9ccb795 into latest Oct 2, 2019
@pjlee11 pjlee11 deleted the remove-scrolling-no-for-a11y-reasons branch October 2, 2019 12:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y Accessibility-related task articles-av-epic ws-articles Tasks for the WS Articles Team ws-media The World Service media stream ws-media- LiveRadioV1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants