From 0fe8d60293157161fd8ab7141859452718cb6ad2 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 12:57:31 +0100 Subject: [PATCH 01/13] add title prop and remove scrolling='no' --- .../components/psammead-media-player/CHANGELOG.md | 1 + .../components/psammead-media-player/README.md | 9 +++++++-- .../psammead-media-player/src/Amp/index.jsx | 4 +++- .../Canonical/__snapshots__/index.test.jsx.snap | 1 - .../psammead-media-player/src/Canonical/index.jsx | 5 +++-- .../src/__snapshots__/index.test.jsx.snap | 2 -- .../psammead-media-player/src/index.jsx | 15 ++++++++++++--- 7 files changed, 26 insertions(+), 11 deletions(-) diff --git a/packages/components/psammead-media-player/CHANGELOG.md b/packages/components/psammead-media-player/CHANGELOG.md index 07d8600631..5246f46a0b 100644 --- a/packages/components/psammead-media-player/CHANGELOG.md +++ b/packages/components/psammead-media-player/CHANGELOG.md @@ -3,6 +3,7 @@ | Version | Description | |---------|-------------| +| 2.0.0 | [PR#xxx](https://github.com/bbc/psammead/pull/xxx) A11y best practice changes, remove alpha and add new required prop `title` | | 1.1.1-alpha.2 | [PR#2247](https://github.com/bbc/psammead/pull/2247) Update article embed URLs in MediaPLayer stories | | 1.1.1-alpha.1 | [PR#2191](https://github.com/bbc/psammead/pull/2191) Talos - Bump Dependencies | | 1.1.0-alpha.1 | [PR#2144](https://github.com/bbc/psammead/pull/2144) Add `skin` prop | diff --git a/packages/components/psammead-media-player/README.md b/packages/components/psammead-media-player/README.md index 132e4389d9..d9039ee0e2 100644 --- a/packages/components/psammead-media-player/README.md +++ b/packages/components/psammead-media-player/README.md @@ -23,11 +23,13 @@ This component to be used at any point on the page, specifically when a media pl | Argument | Type | Required | Default | Example | |-----------|---------------------|----------|---------|-----------------| | `src` | string | Yes | - | `http://foobar.com/embeddable_endpoint` | +| `title` | string | Yes | - | `Video - description of video` | | `showPlaceholder` | boolean | No | `true` | `false` | | `placeholderSrc` | string | No | `null` | `http://foobar.com/placeholder.png` | | `portrait` | boolean | No | `false` | `true` | The `src` prop is required, as it tells the component what page it needs to embed. +The `title` prop is required, as it is part of a11y best practices. The `portrait` prop is not required, and defaults to `false`. This is to support portrait video content in the future. The `showPlaceholder` boolean prop is also not required, and defaults to `true`. Assuming `showPlaceholder` is `true`, the `placeholderSrc` will be what image to display as the placeholder. @@ -37,6 +39,7 @@ Assuming `showPlaceholder` is `true`, the `placeholderSrc` will be what image to | Argument | Type | Required | Default | Example | |-----------|---------------------|----------|---------|-----------------| | `src` | string | Yes | - | `http://foobar.com/embeddable_endpoint` | +| `title` | string | Yes | - | `Video - description of video` | | `portrait` | boolean | No | `false` | `true` | | `placeholderSrc` | string | yes | - | `http://foobar.com/placeholder.png` | @@ -47,9 +50,10 @@ The `placeholderSrc` prop is required for AMP, as in order to have the component ```js import { CanonicalMediaPlayer } from '@bbc/psammead-media-player'; -const Container = ({ src, portrait, showPlaceholder, placeholderSrc }) => ( +const Container = ({ src, portrait, showPlaceholder, placeholderSrc, title }) => ( ( ```js import { AmpMediaPlayer } from '@bbc/psammead-media-player'; -const Container = ({ src, portrait, placeholderSrc }) => ( +const Container = ({ src, portrait, placeholderSrc, title }) => ( diff --git a/packages/components/psammead-media-player/src/Amp/index.jsx b/packages/components/psammead-media-player/src/Amp/index.jsx index 921c54562f..b366265137 100644 --- a/packages/components/psammead-media-player/src/Amp/index.jsx +++ b/packages/components/psammead-media-player/src/Amp/index.jsx @@ -12,12 +12,13 @@ const AmpHead = () => ( ); -const AmpMediaPlayer = ({ src, placeholderSrc }) => { +const AmpMediaPlayer = ({ src, placeholderSrc, title }) => { return ( <> { AmpMediaPlayer.propTypes = { src: string.isRequired, placeholderSrc: string.isRequired, + title: string.isRequired, }; export default AmpMediaPlayer; diff --git a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap index de66fe4189..0c823cd89e 100644 --- a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap @@ -15,7 +15,6 @@ exports[`Media Player: Canonical should render an iframe 1`] = ` allow="autoplay; fullscreen" allowfullscreen="" class="c0" - scrolling="no" src="https://foo.bar/iframe" /> `; diff --git a/packages/components/psammead-media-player/src/Canonical/index.jsx b/packages/components/psammead-media-player/src/Canonical/index.jsx index 60a5f4bf02..8b016ac0a5 100644 --- a/packages/components/psammead-media-player/src/Canonical/index.jsx +++ b/packages/components/psammead-media-player/src/Canonical/index.jsx @@ -12,10 +12,10 @@ const StyledIframe = styled.iframe` overflow: hidden; `; -const Canonical = ({ src }) => ( +const Canonical = ({ src, title }) => ( ( Canonical.propTypes = { src: string.isRequired, + title: string.isRequired, }; export default Canonical; diff --git a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap index 96a3a4c129..e18bda8e66 100644 --- a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap @@ -214,7 +214,6 @@ exports[`Media Player: Canonical Entry renders an iframe when showPlaceholder is allow="autoplay; fullscreen" allowfullscreen="" class="c1" - scrolling="no" src="http://foo.bar/iframe" /> @@ -244,7 +243,6 @@ exports[`Media Player: Canonical Entry renders the audio skin 1`] = ` allow="autoplay; fullscreen" allowfullscreen="" class="c1" - scrolling="no" src="https://www.test.bbc.com/ws/av-embeds/media/bbc_korean_radio/liveradio" /> diff --git a/packages/components/psammead-media-player/src/index.jsx b/packages/components/psammead-media-player/src/index.jsx index daf3d5b965..de7e2c9538 100644 --- a/packages/components/psammead-media-player/src/index.jsx +++ b/packages/components/psammead-media-player/src/index.jsx @@ -25,6 +25,7 @@ export const CanonicalMediaPlayer = ({ portrait, src, skin, + title, }) => { const [placeholderActive, setPlaceholderActive] = useState(showPlaceholder); const handlePlaceholderClick = () => setPlaceholderActive(false); @@ -37,19 +38,25 @@ export const CanonicalMediaPlayer = ({ {placeholderActive ? ( ) : ( - + )} ); }; -export const AmpMediaPlayer = ({ placeholderSrc, portrait, src, skin }) => { +export const AmpMediaPlayer = ({ + placeholderSrc, + portrait, + src, + skin, + title, +}) => { const StyledContainer = skin === 'audio' ? StyledAudioContainer : StyledVideoContainer; return ( - + ); }; @@ -59,6 +66,7 @@ CanonicalMediaPlayer.propTypes = { portrait: bool, showPlaceholder: bool, src: string.isRequired, + title: string.isRequired, skin: oneOf(['classic', 'audio']), }; @@ -73,6 +81,7 @@ AmpMediaPlayer.propTypes = { placeholderSrc: string.isRequired, portrait: bool, src: string.isRequired, + title: string.isRequired, skin: oneOf(['classic', 'audio']), }; From 7d61146d02c2e1e01686b596b0a855a6fa9fd09f Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 13:12:40 +0100 Subject: [PATCH 02/13] bump to 2.0.0 --- packages/components/psammead-media-player/package-lock.json | 2 +- packages/components/psammead-media-player/package.json | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/components/psammead-media-player/package-lock.json b/packages/components/psammead-media-player/package-lock.json index 6eb07a3b24..142773e449 100644 --- a/packages/components/psammead-media-player/package-lock.json +++ b/packages/components/psammead-media-player/package-lock.json @@ -1,6 +1,6 @@ { "name": "@bbc/psammead-media-player", - "version": "1.1.1-alpha.2", + "version": "2.0.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/packages/components/psammead-media-player/package.json b/packages/components/psammead-media-player/package.json index 9a6fe25ba4..df53b68a62 100644 --- a/packages/components/psammead-media-player/package.json +++ b/packages/components/psammead-media-player/package.json @@ -1,9 +1,6 @@ { "name": "@bbc/psammead-media-player", - "version": "1.1.1-alpha.2", - "publishConfig": { - "tag": "alpha" - }, + "version": "2.0.0", "description": "Provides a media player with optional placeholder", "main": "dist/index.js", "module": "esm/index.js", From f7265215c39855db10bda50ec93049d3b7e85172 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 13:12:55 +0100 Subject: [PATCH 03/13] remove alpha references --- packages/components/psammead-media-player/README.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/components/psammead-media-player/README.md b/packages/components/psammead-media-player/README.md index d9039ee0e2..3e5f82bbbc 100644 --- a/packages/components/psammead-media-player/README.md +++ b/packages/components/psammead-media-player/README.md @@ -1,7 +1,3 @@ -# ⛔️ This is an alpha component ⛔️ - -This component is currently tagged as alpha and is not suitable for production use. Following the passing of an accessibility review this component will be marked as ready for production and the alpha tag removed. - # psammead-media-player · [![Known Vulnerabilities](https://snyk.io/test/github/bbc/psammead/badge.svg?targetFile=packages%2Fcomponents%2Fpsammead-brand%2Fpackage.json)](https://snyk.io/test/github/bbc/psammead?targetFile=packages%2Fcomponents%2Fpsammead-brand%2Fpackage.json) [![Dependency Status](https://david-dm.org/bbc/psammead.svg?path=packages/components/psammead-media-player)](https://david-dm.org/bbc/psammead?path=packages/components/psammead-media-player) [![peerDependencies Status](https://david-dm.org/bbc/psammead/peer-status.svg?path=packages/components/psammead-media-player)](https://david-dm.org/bbc/psammead?path=packages/components/psammead-media-player&type=peer) [![Storybook](https://raw.githubusercontent.com/storybooks/brand/master/badge/badge-storybook.svg?sanitize=true)](https://bbc.github.io/psammead/?path=/story/brand--default) [![GitHub license](https://img.shields.io/badge/license-Apache%202.0-blue.svg)](https://github.com/bbc/psammead/blob/latest/LICENSE) [![npm version](https://img.shields.io/npm/v/@bbc/psammead-media-player.svg)](https://www.npmjs.com/package/@bbc/psammead-media-player) [![PRs Welcome](https://img.shields.io/badge/PRs-welcome-brightgreen.svg)](https://github.com/bbc/psammead/blob/latest/CONTRIBUTING.md) ## Description @@ -76,7 +72,7 @@ const Container = ({ src, portrait, placeholderSrc, title }) => ( ``` ## Accessibility notes -This component is still in its initial alpha stages, and requires a full and extensive accessibility review. +This component requires a `title` value for accessibilty best practices. It also should not have `scrolling="no"` added in the future as this is against a11y best practices. ## Contributing From ad25a2a11dd5c79fdc5dbf5b0a590def0f7fd74a Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 13:13:12 +0100 Subject: [PATCH 04/13] add title prop to stories and test --- .../src/Amp/__snapshots__/index.test.jsx.snap | 1 + .../psammead-media-player/src/Amp/index.test.jsx | 1 + .../Canonical/__snapshots__/index.test.jsx.snap | 1 + .../src/Canonical/index.test.jsx | 2 +- .../src/__snapshots__/index.test.jsx.snap | 5 +++++ .../psammead-media-player/src/index.stories.jsx | 16 +++++++++++++++- .../psammead-media-player/src/index.test.jsx | 7 +++++++ 7 files changed, 31 insertions(+), 2 deletions(-) diff --git a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap index b8ab2d532f..44fb66be8b 100644 --- a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap @@ -17,6 +17,7 @@ exports[`Media Player: Amp should render an amp-iframe with an amp-img nested in layout="fill" sandbox="allow-scripts allow-same-origin" src="https://foo.bar/iframe" + title="An iframe" > { , ); }); diff --git a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap index 0c823cd89e..92b25e6fb5 100644 --- a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap @@ -16,5 +16,6 @@ exports[`Media Player: Canonical should render an iframe 1`] = ` allowfullscreen="" class="c0" src="https://foo.bar/iframe" + title="An iframe" /> `; diff --git a/packages/components/psammead-media-player/src/Canonical/index.test.jsx b/packages/components/psammead-media-player/src/Canonical/index.test.jsx index 20f1d1dcd9..1b28f96ad3 100644 --- a/packages/components/psammead-media-player/src/Canonical/index.test.jsx +++ b/packages/components/psammead-media-player/src/Canonical/index.test.jsx @@ -5,6 +5,6 @@ import Canonical from '.'; describe('Media Player: Canonical', () => { shouldMatchSnapshot( 'should render an iframe', - , + , ); }); diff --git a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap index e18bda8e66..00106b0f55 100644 --- a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap @@ -26,6 +26,7 @@ exports[`Media Player: AMP Entry renders a landscape container with an amp-ifram layout="fill" sandbox="allow-scripts allow-same-origin" src="http://foo.bar/iframe/amp" + title="An iframe" > `; @@ -244,6 +248,7 @@ exports[`Media Player: Canonical Entry renders the audio skin 1`] = ` allowfullscreen="" class="c1" src="https://www.test.bbc.com/ws/av-embeds/media/bbc_korean_radio/liveradio" + title="An iframe" /> `; diff --git a/packages/components/psammead-media-player/src/index.stories.jsx b/packages/components/psammead-media-player/src/index.stories.jsx index a2d903b898..80960ad61e 100644 --- a/packages/components/psammead-media-player/src/index.stories.jsx +++ b/packages/components/psammead-media-player/src/index.stories.jsx @@ -6,6 +6,7 @@ import { ampDecorator } from '../../../../.storybook/config'; storiesOf('Components|Media Player', module).add('default', () => ( )); @@ -15,7 +16,8 @@ storiesOf('Components|Media Player', module) .add('AMP', () => ( )); @@ -23,7 +25,19 @@ storiesOf('Components|Media Player', module) storiesOf('Components|Media Player', module).add('Audio Skin', () => ( )); + +storiesOf('Components|Media Player', module) + .addDecorator(ampDecorator) + .add('AMP Audio skin', () => ( + + )); diff --git a/packages/components/psammead-media-player/src/index.test.jsx b/packages/components/psammead-media-player/src/index.test.jsx index 733f4aab14..632928bc0b 100644 --- a/packages/components/psammead-media-player/src/index.test.jsx +++ b/packages/components/psammead-media-player/src/index.test.jsx @@ -8,6 +8,7 @@ describe('Media Player: AMP Entry', () => { , ); @@ -17,6 +18,7 @@ describe('Media Player: AMP Entry', () => { portrait placeholderSrc="http://foo.bar/placeholder.png" src="http://foo.bar/iframe/amp" + title="An iframe" />, ); @@ -25,6 +27,7 @@ describe('Media Player: AMP Entry', () => { , ); @@ -36,6 +39,7 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -44,6 +48,7 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -53,6 +58,7 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -61,6 +67,7 @@ describe('Media Player: Canonical Entry', () => { , ); From 0e9b20bdb97c00bfc8ae86b6604edb37a1265355 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 13:31:14 +0100 Subject: [PATCH 05/13] PR number --- packages/components/psammead-media-player/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/psammead-media-player/CHANGELOG.md b/packages/components/psammead-media-player/CHANGELOG.md index 5246f46a0b..75aa723c3b 100644 --- a/packages/components/psammead-media-player/CHANGELOG.md +++ b/packages/components/psammead-media-player/CHANGELOG.md @@ -3,7 +3,7 @@ | Version | Description | |---------|-------------| -| 2.0.0 | [PR#xxx](https://github.com/bbc/psammead/pull/xxx) A11y best practice changes, remove alpha and add new required prop `title` | +| 2.0.0 | [PR#2280](https://github.com/bbc/psammead/pull/2280) A11y best practice changes, remove alpha and add new required prop `title` | | 1.1.1-alpha.2 | [PR#2247](https://github.com/bbc/psammead/pull/2247) Update article embed URLs in MediaPLayer stories | | 1.1.1-alpha.1 | [PR#2191](https://github.com/bbc/psammead/pull/2191) Talos - Bump Dependencies | | 1.1.0-alpha.1 | [PR#2144](https://github.com/bbc/psammead/pull/2144) Add `skin` prop | From 1ebd47606dcbae3ffbc6755455060d57f39c7b1e Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 13:13:12 +0100 Subject: [PATCH 06/13] add title prop to stories and test --- .../src/Amp/__snapshots__/index.test.jsx.snap | 1 + .../psammead-media-player/src/Amp/index.test.jsx | 1 + .../Canonical/__snapshots__/index.test.jsx.snap | 1 + .../src/Canonical/index.test.jsx | 2 +- .../src/__snapshots__/index.test.jsx.snap | 5 +++++ .../psammead-media-player/src/index.stories.jsx | 16 +++++++++++++++- .../psammead-media-player/src/index.test.jsx | 7 +++++++ 7 files changed, 31 insertions(+), 2 deletions(-) diff --git a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap index b8ab2d532f..44fb66be8b 100644 --- a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap @@ -17,6 +17,7 @@ exports[`Media Player: Amp should render an amp-iframe with an amp-img nested in layout="fill" sandbox="allow-scripts allow-same-origin" src="https://foo.bar/iframe" + title="An iframe" > { , ); }); diff --git a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap index 0c823cd89e..92b25e6fb5 100644 --- a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap @@ -16,5 +16,6 @@ exports[`Media Player: Canonical should render an iframe 1`] = ` allowfullscreen="" class="c0" src="https://foo.bar/iframe" + title="An iframe" /> `; diff --git a/packages/components/psammead-media-player/src/Canonical/index.test.jsx b/packages/components/psammead-media-player/src/Canonical/index.test.jsx index 20f1d1dcd9..1b28f96ad3 100644 --- a/packages/components/psammead-media-player/src/Canonical/index.test.jsx +++ b/packages/components/psammead-media-player/src/Canonical/index.test.jsx @@ -5,6 +5,6 @@ import Canonical from '.'; describe('Media Player: Canonical', () => { shouldMatchSnapshot( 'should render an iframe', - , + , ); }); diff --git a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap index e18bda8e66..00106b0f55 100644 --- a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap @@ -26,6 +26,7 @@ exports[`Media Player: AMP Entry renders a landscape container with an amp-ifram layout="fill" sandbox="allow-scripts allow-same-origin" src="http://foo.bar/iframe/amp" + title="An iframe" > `; @@ -244,6 +248,7 @@ exports[`Media Player: Canonical Entry renders the audio skin 1`] = ` allowfullscreen="" class="c1" src="https://www.test.bbc.com/ws/av-embeds/media/bbc_korean_radio/liveradio" + title="An iframe" /> `; diff --git a/packages/components/psammead-media-player/src/index.stories.jsx b/packages/components/psammead-media-player/src/index.stories.jsx index a2d903b898..80960ad61e 100644 --- a/packages/components/psammead-media-player/src/index.stories.jsx +++ b/packages/components/psammead-media-player/src/index.stories.jsx @@ -6,6 +6,7 @@ import { ampDecorator } from '../../../../.storybook/config'; storiesOf('Components|Media Player', module).add('default', () => ( )); @@ -15,7 +16,8 @@ storiesOf('Components|Media Player', module) .add('AMP', () => ( )); @@ -23,7 +25,19 @@ storiesOf('Components|Media Player', module) storiesOf('Components|Media Player', module).add('Audio Skin', () => ( )); + +storiesOf('Components|Media Player', module) + .addDecorator(ampDecorator) + .add('AMP Audio skin', () => ( + + )); diff --git a/packages/components/psammead-media-player/src/index.test.jsx b/packages/components/psammead-media-player/src/index.test.jsx index 733f4aab14..632928bc0b 100644 --- a/packages/components/psammead-media-player/src/index.test.jsx +++ b/packages/components/psammead-media-player/src/index.test.jsx @@ -8,6 +8,7 @@ describe('Media Player: AMP Entry', () => { , ); @@ -17,6 +18,7 @@ describe('Media Player: AMP Entry', () => { portrait placeholderSrc="http://foo.bar/placeholder.png" src="http://foo.bar/iframe/amp" + title="An iframe" />, ); @@ -25,6 +27,7 @@ describe('Media Player: AMP Entry', () => { , ); @@ -36,6 +39,7 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -44,6 +48,7 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -53,6 +58,7 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -61,6 +67,7 @@ describe('Media Player: Canonical Entry', () => { , ); From c0f7c400e72620734986305b5b5b7827f587f3bc Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 16:05:19 +0100 Subject: [PATCH 07/13] remove title related changes --- .../components/psammead-media-player/CHANGELOG.md | 2 +- .../components/psammead-media-player/README.md | 15 +++++++-------- .../components/psammead-media-player/package.json | 2 +- .../src/Amp/__snapshots__/index.test.jsx.snap | 1 - .../psammead-media-player/src/Amp/index.jsx | 4 +--- .../psammead-media-player/src/Amp/index.test.jsx | 1 - .../Canonical/__snapshots__/index.test.jsx.snap | 1 - .../psammead-media-player/src/Canonical/index.jsx | 4 +--- .../src/Canonical/index.test.jsx | 2 +- .../src/__snapshots__/index.test.jsx.snap | 5 ----- .../psammead-media-player/src/index.jsx | 15 +++------------ .../psammead-media-player/src/index.stories.jsx | 4 ---- .../psammead-media-player/src/index.test.jsx | 7 ------- 13 files changed, 15 insertions(+), 48 deletions(-) diff --git a/packages/components/psammead-media-player/CHANGELOG.md b/packages/components/psammead-media-player/CHANGELOG.md index 5246f46a0b..cc9a3c4edc 100644 --- a/packages/components/psammead-media-player/CHANGELOG.md +++ b/packages/components/psammead-media-player/CHANGELOG.md @@ -3,7 +3,7 @@ | Version | Description | |---------|-------------| -| 2.0.0 | [PR#xxx](https://github.com/bbc/psammead/pull/xxx) A11y best practice changes, remove alpha and add new required prop `title` | +| 1.1.1-alpha.3 | [PR#2280](https://github.com/bbc/psammead/pull/2280) Remove `scrolling="no"` for a11y best practice | | 1.1.1-alpha.2 | [PR#2247](https://github.com/bbc/psammead/pull/2247) Update article embed URLs in MediaPLayer stories | | 1.1.1-alpha.1 | [PR#2191](https://github.com/bbc/psammead/pull/2191) Talos - Bump Dependencies | | 1.1.0-alpha.1 | [PR#2144](https://github.com/bbc/psammead/pull/2144) Add `skin` prop | diff --git a/packages/components/psammead-media-player/README.md b/packages/components/psammead-media-player/README.md index 3e5f82bbbc..132e4389d9 100644 --- a/packages/components/psammead-media-player/README.md +++ b/packages/components/psammead-media-player/README.md @@ -1,3 +1,7 @@ +# ⛔️ This is an alpha component ⛔️ + +This component is currently tagged as alpha and is not suitable for production use. Following the passing of an accessibility review this component will be marked as ready for production and the alpha tag removed. + # psammead-media-player · [![Known Vulnerabilities](https://snyk.io/test/github/bbc/psammead/badge.svg?targetFile=packages%2Fcomponents%2Fpsammead-brand%2Fpackage.json)](https://snyk.io/test/github/bbc/psammead?targetFile=packages%2Fcomponents%2Fpsammead-brand%2Fpackage.json) [![Dependency Status](https://david-dm.org/bbc/psammead.svg?path=packages/components/psammead-media-player)](https://david-dm.org/bbc/psammead?path=packages/components/psammead-media-player) [![peerDependencies Status](https://david-dm.org/bbc/psammead/peer-status.svg?path=packages/components/psammead-media-player)](https://david-dm.org/bbc/psammead?path=packages/components/psammead-media-player&type=peer) [![Storybook](https://raw.githubusercontent.com/storybooks/brand/master/badge/badge-storybook.svg?sanitize=true)](https://bbc.github.io/psammead/?path=/story/brand--default) [![GitHub license](https://img.shields.io/badge/license-Apache%202.0-blue.svg)](https://github.com/bbc/psammead/blob/latest/LICENSE) [![npm version](https://img.shields.io/npm/v/@bbc/psammead-media-player.svg)](https://www.npmjs.com/package/@bbc/psammead-media-player) [![PRs Welcome](https://img.shields.io/badge/PRs-welcome-brightgreen.svg)](https://github.com/bbc/psammead/blob/latest/CONTRIBUTING.md) ## Description @@ -19,13 +23,11 @@ This component to be used at any point on the page, specifically when a media pl | Argument | Type | Required | Default | Example | |-----------|---------------------|----------|---------|-----------------| | `src` | string | Yes | - | `http://foobar.com/embeddable_endpoint` | -| `title` | string | Yes | - | `Video - description of video` | | `showPlaceholder` | boolean | No | `true` | `false` | | `placeholderSrc` | string | No | `null` | `http://foobar.com/placeholder.png` | | `portrait` | boolean | No | `false` | `true` | The `src` prop is required, as it tells the component what page it needs to embed. -The `title` prop is required, as it is part of a11y best practices. The `portrait` prop is not required, and defaults to `false`. This is to support portrait video content in the future. The `showPlaceholder` boolean prop is also not required, and defaults to `true`. Assuming `showPlaceholder` is `true`, the `placeholderSrc` will be what image to display as the placeholder. @@ -35,7 +37,6 @@ Assuming `showPlaceholder` is `true`, the `placeholderSrc` will be what image to | Argument | Type | Required | Default | Example | |-----------|---------------------|----------|---------|-----------------| | `src` | string | Yes | - | `http://foobar.com/embeddable_endpoint` | -| `title` | string | Yes | - | `Video - description of video` | | `portrait` | boolean | No | `false` | `true` | | `placeholderSrc` | string | yes | - | `http://foobar.com/placeholder.png` | @@ -46,10 +47,9 @@ The `placeholderSrc` prop is required for AMP, as in order to have the component ```js import { CanonicalMediaPlayer } from '@bbc/psammead-media-player'; -const Container = ({ src, portrait, showPlaceholder, placeholderSrc, title }) => ( +const Container = ({ src, portrait, showPlaceholder, placeholderSrc }) => ( ```js import { AmpMediaPlayer } from '@bbc/psammead-media-player'; -const Container = ({ src, portrait, placeholderSrc, title }) => ( +const Container = ({ src, portrait, placeholderSrc }) => ( @@ -72,7 +71,7 @@ const Container = ({ src, portrait, placeholderSrc, title }) => ( ``` ## Accessibility notes -This component requires a `title` value for accessibilty best practices. It also should not have `scrolling="no"` added in the future as this is against a11y best practices. +This component is still in its initial alpha stages, and requires a full and extensive accessibility review. ## Contributing diff --git a/packages/components/psammead-media-player/package.json b/packages/components/psammead-media-player/package.json index df53b68a62..6b0c3ac9d3 100644 --- a/packages/components/psammead-media-player/package.json +++ b/packages/components/psammead-media-player/package.json @@ -1,6 +1,6 @@ { "name": "@bbc/psammead-media-player", - "version": "2.0.0", + "version": "1.1.1-alpha.3", "description": "Provides a media player with optional placeholder", "main": "dist/index.js", "module": "esm/index.js", diff --git a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap index 44fb66be8b..b8ab2d532f 100644 --- a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap @@ -17,7 +17,6 @@ exports[`Media Player: Amp should render an amp-iframe with an amp-img nested in layout="fill" sandbox="allow-scripts allow-same-origin" src="https://foo.bar/iframe" - title="An iframe" > ( ); -const AmpMediaPlayer = ({ src, placeholderSrc, title }) => { +const AmpMediaPlayer = ({ src, placeholderSrc }) => { return ( <> { AmpMediaPlayer.propTypes = { src: string.isRequired, placeholderSrc: string.isRequired, - title: string.isRequired, }; export default AmpMediaPlayer; diff --git a/packages/components/psammead-media-player/src/Amp/index.test.jsx b/packages/components/psammead-media-player/src/Amp/index.test.jsx index e8dba8c533..d688d376c7 100644 --- a/packages/components/psammead-media-player/src/Amp/index.test.jsx +++ b/packages/components/psammead-media-player/src/Amp/index.test.jsx @@ -8,7 +8,6 @@ describe('Media Player: Amp', () => { , ); }); diff --git a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap index 92b25e6fb5..0c823cd89e 100644 --- a/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Canonical/__snapshots__/index.test.jsx.snap @@ -16,6 +16,5 @@ exports[`Media Player: Canonical should render an iframe 1`] = ` allowfullscreen="" class="c0" src="https://foo.bar/iframe" - title="An iframe" /> `; diff --git a/packages/components/psammead-media-player/src/Canonical/index.jsx b/packages/components/psammead-media-player/src/Canonical/index.jsx index 8b016ac0a5..5fce230df7 100644 --- a/packages/components/psammead-media-player/src/Canonical/index.jsx +++ b/packages/components/psammead-media-player/src/Canonical/index.jsx @@ -12,10 +12,9 @@ const StyledIframe = styled.iframe` overflow: hidden; `; -const Canonical = ({ src, title }) => ( +const Canonical = ({ src }) => ( ( Canonical.propTypes = { src: string.isRequired, - title: string.isRequired, }; export default Canonical; diff --git a/packages/components/psammead-media-player/src/Canonical/index.test.jsx b/packages/components/psammead-media-player/src/Canonical/index.test.jsx index 1b28f96ad3..20f1d1dcd9 100644 --- a/packages/components/psammead-media-player/src/Canonical/index.test.jsx +++ b/packages/components/psammead-media-player/src/Canonical/index.test.jsx @@ -5,6 +5,6 @@ import Canonical from '.'; describe('Media Player: Canonical', () => { shouldMatchSnapshot( 'should render an iframe', - , + , ); }); diff --git a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap index 00106b0f55..e18bda8e66 100644 --- a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap @@ -26,7 +26,6 @@ exports[`Media Player: AMP Entry renders a landscape container with an amp-ifram layout="fill" sandbox="allow-scripts allow-same-origin" src="http://foo.bar/iframe/amp" - title="An iframe" > `; @@ -248,7 +244,6 @@ exports[`Media Player: Canonical Entry renders the audio skin 1`] = ` allowfullscreen="" class="c1" src="https://www.test.bbc.com/ws/av-embeds/media/bbc_korean_radio/liveradio" - title="An iframe" /> `; diff --git a/packages/components/psammead-media-player/src/index.jsx b/packages/components/psammead-media-player/src/index.jsx index de7e2c9538..daf3d5b965 100644 --- a/packages/components/psammead-media-player/src/index.jsx +++ b/packages/components/psammead-media-player/src/index.jsx @@ -25,7 +25,6 @@ export const CanonicalMediaPlayer = ({ portrait, src, skin, - title, }) => { const [placeholderActive, setPlaceholderActive] = useState(showPlaceholder); const handlePlaceholderClick = () => setPlaceholderActive(false); @@ -38,25 +37,19 @@ export const CanonicalMediaPlayer = ({ {placeholderActive ? ( ) : ( - + )} ); }; -export const AmpMediaPlayer = ({ - placeholderSrc, - portrait, - src, - skin, - title, -}) => { +export const AmpMediaPlayer = ({ placeholderSrc, portrait, src, skin }) => { const StyledContainer = skin === 'audio' ? StyledAudioContainer : StyledVideoContainer; return ( - + ); }; @@ -66,7 +59,6 @@ CanonicalMediaPlayer.propTypes = { portrait: bool, showPlaceholder: bool, src: string.isRequired, - title: string.isRequired, skin: oneOf(['classic', 'audio']), }; @@ -81,7 +73,6 @@ AmpMediaPlayer.propTypes = { placeholderSrc: string.isRequired, portrait: bool, src: string.isRequired, - title: string.isRequired, skin: oneOf(['classic', 'audio']), }; diff --git a/packages/components/psammead-media-player/src/index.stories.jsx b/packages/components/psammead-media-player/src/index.stories.jsx index 80960ad61e..fbee93e007 100644 --- a/packages/components/psammead-media-player/src/index.stories.jsx +++ b/packages/components/psammead-media-player/src/index.stories.jsx @@ -6,7 +6,6 @@ import { ampDecorator } from '../../../../.storybook/config'; storiesOf('Components|Media Player', module).add('default', () => ( )); @@ -17,7 +16,6 @@ storiesOf('Components|Media Player', module) )); @@ -25,7 +23,6 @@ storiesOf('Components|Media Player', module) storiesOf('Components|Media Player', module).add('Audio Skin', () => ( @@ -37,7 +34,6 @@ storiesOf('Components|Media Player', module) )); diff --git a/packages/components/psammead-media-player/src/index.test.jsx b/packages/components/psammead-media-player/src/index.test.jsx index 632928bc0b..733f4aab14 100644 --- a/packages/components/psammead-media-player/src/index.test.jsx +++ b/packages/components/psammead-media-player/src/index.test.jsx @@ -8,7 +8,6 @@ describe('Media Player: AMP Entry', () => { , ); @@ -18,7 +17,6 @@ describe('Media Player: AMP Entry', () => { portrait placeholderSrc="http://foo.bar/placeholder.png" src="http://foo.bar/iframe/amp" - title="An iframe" />, ); @@ -27,7 +25,6 @@ describe('Media Player: AMP Entry', () => { , ); @@ -39,7 +36,6 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -48,7 +44,6 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -58,7 +53,6 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -67,7 +61,6 @@ describe('Media Player: Canonical Entry', () => { , ); From 4002710e7318b2e8f7d0d95ee7304cd8e2785e92 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 16:13:53 +0100 Subject: [PATCH 08/13] update snapshots --- .../src/Amp/__snapshots__/index.test.jsx.snap | 1 - .../src/Canonical/__snapshots__/index.test.jsx.snap | 1 - .../src/__snapshots__/index.test.jsx.snap | 5 ----- 3 files changed, 7 deletions(-) diff --git a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap index 44fb66be8b..b8ab2d532f 100644 --- a/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/Amp/__snapshots__/index.test.jsx.snap @@ -17,7 +17,6 @@ exports[`Media Player: Amp should render an amp-iframe with an amp-img nested in layout="fill" sandbox="allow-scripts allow-same-origin" src="https://foo.bar/iframe" - title="An iframe" > `; diff --git a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap index 00106b0f55..e18bda8e66 100644 --- a/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap +++ b/packages/components/psammead-media-player/src/__snapshots__/index.test.jsx.snap @@ -26,7 +26,6 @@ exports[`Media Player: AMP Entry renders a landscape container with an amp-ifram layout="fill" sandbox="allow-scripts allow-same-origin" src="http://foo.bar/iframe/amp" - title="An iframe" > `; @@ -248,7 +244,6 @@ exports[`Media Player: Canonical Entry renders the audio skin 1`] = ` allowfullscreen="" class="c1" src="https://www.test.bbc.com/ws/av-embeds/media/bbc_korean_radio/liveradio" - title="An iframe" /> `; From 0b4b2551c01ce9569e01ec76ae80e40b9d552640 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 16:16:45 +0100 Subject: [PATCH 09/13] npm install package-lock --- packages/components/psammead-media-player/package-lock.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/psammead-media-player/package-lock.json b/packages/components/psammead-media-player/package-lock.json index 142773e449..70c2648ba0 100644 --- a/packages/components/psammead-media-player/package-lock.json +++ b/packages/components/psammead-media-player/package-lock.json @@ -1,6 +1,6 @@ { "name": "@bbc/psammead-media-player", - "version": "2.0.0", + "version": "1.1.1-alpha.3", "lockfileVersion": 1, "requires": true, "dependencies": { From 3796df14de9caf3b3a701ec4945836fdaf55f47c Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 16:17:03 +0100 Subject: [PATCH 10/13] remove title --- packages/components/psammead-media-player/src/Amp/index.test.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/components/psammead-media-player/src/Amp/index.test.jsx b/packages/components/psammead-media-player/src/Amp/index.test.jsx index e8dba8c533..d688d376c7 100644 --- a/packages/components/psammead-media-player/src/Amp/index.test.jsx +++ b/packages/components/psammead-media-player/src/Amp/index.test.jsx @@ -8,7 +8,6 @@ describe('Media Player: Amp', () => { , ); }); From 6448abcb3eda774390451644381c503c521402c6 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 16:17:54 +0100 Subject: [PATCH 11/13] remove title --- .../components/psammead-media-player/src/index.test.jsx | 7 ------- 1 file changed, 7 deletions(-) diff --git a/packages/components/psammead-media-player/src/index.test.jsx b/packages/components/psammead-media-player/src/index.test.jsx index 632928bc0b..733f4aab14 100644 --- a/packages/components/psammead-media-player/src/index.test.jsx +++ b/packages/components/psammead-media-player/src/index.test.jsx @@ -8,7 +8,6 @@ describe('Media Player: AMP Entry', () => { , ); @@ -18,7 +17,6 @@ describe('Media Player: AMP Entry', () => { portrait placeholderSrc="http://foo.bar/placeholder.png" src="http://foo.bar/iframe/amp" - title="An iframe" />, ); @@ -27,7 +25,6 @@ describe('Media Player: AMP Entry', () => { , ); @@ -39,7 +36,6 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -48,7 +44,6 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -58,7 +53,6 @@ describe('Media Player: Canonical Entry', () => { , ); @@ -67,7 +61,6 @@ describe('Media Player: Canonical Entry', () => { , ); From d7ecdfe95273fb8b35b430645f01b7cc946d0aef Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 16:18:41 +0100 Subject: [PATCH 12/13] alpha tag --- packages/components/psammead-media-player/package.json | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/components/psammead-media-player/package.json b/packages/components/psammead-media-player/package.json index 6b0c3ac9d3..8b33788199 100644 --- a/packages/components/psammead-media-player/package.json +++ b/packages/components/psammead-media-player/package.json @@ -1,6 +1,9 @@ { "name": "@bbc/psammead-media-player", "version": "1.1.1-alpha.3", + "publishConfig": { + "tag": "alpha" + }, "description": "Provides a media player with optional placeholder", "main": "dist/index.js", "module": "esm/index.js", From 0499f7cec8ea51279fd27ce613d27800c033ceef Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 1 Oct 2019 16:19:16 +0100 Subject: [PATCH 13/13] remove title --- .../psammead-media-player/src/Canonical/index.test.jsx | 2 +- packages/components/psammead-media-player/src/index.stories.jsx | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/components/psammead-media-player/src/Canonical/index.test.jsx b/packages/components/psammead-media-player/src/Canonical/index.test.jsx index 1b28f96ad3..20f1d1dcd9 100644 --- a/packages/components/psammead-media-player/src/Canonical/index.test.jsx +++ b/packages/components/psammead-media-player/src/Canonical/index.test.jsx @@ -5,6 +5,6 @@ import Canonical from '.'; describe('Media Player: Canonical', () => { shouldMatchSnapshot( 'should render an iframe', - , + , ); }); diff --git a/packages/components/psammead-media-player/src/index.stories.jsx b/packages/components/psammead-media-player/src/index.stories.jsx index a072b896a4..fbee93e007 100644 --- a/packages/components/psammead-media-player/src/index.stories.jsx +++ b/packages/components/psammead-media-player/src/index.stories.jsx @@ -23,7 +23,6 @@ storiesOf('Components|Media Player', module) storiesOf('Components|Media Player', module).add('Audio Skin', () => (