Fix prop type warnings #2516
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,26 @@ | ||
| # Psammead Media Player Changelog | ||
|
|
||
| <!-- prettier-ignore --> | ||
| | Version | Description | | ||
| | ------- | ----------- | | ||
| | 2.1.2 | [PR#2516](https://github.com/bbc/psammead/pull/2516) Add notes to stories. Add default `alt`, `height` and `width` values | | ||
| | 2.1.1 | [PR#2532](https://github.com/bbc/psammead/pull/2532) Rename "Media Player" story to "MediaPlayer" | ||
| | 2.1.0 | [PR#2424](https://github.com/bbc/psammead/pull/2424) Add srcset to placeholder image | ||
| | 2.0.2 | [PR#2486](https://github.com/bbc/psammead/pull/2486) Talos - Bump Dependencies - @bbc/psammead-image | | ||
| | 2.0.1 | [PR#2317](https://github.com/bbc/psammead/pull/2317) Add `psammead-play-button` to Media Player placeholder | | ||
| | 2.0.0 | [PR#2434](https://github.com/bbc/psammead/pull/2434) Add `title` attribute to AV media player | | ||
| | 1.1.1-alpha.8 | [PR#2410](https://github.com/bbc/psammead/pull/2410) Update AMP story audio skin AMP placeholder image | | ||
| | 1.1.1-alpha.7 | [PR#2312](https://github.com/bbc/psammead/pull/2312) Add lang to the embed src URL in storybook examples | | ||
| | 1.1.1-alpha.6 | [PR#2297](https://github.com/bbc/psammead/pull/2297) define placeholderSrc | | ||
| | 1.1.1-alpha.5 | [PR#2280](https://github.com/bbc/psammead/pull/2280) Remove `scrolling="no"` for a11y best practice | | ||
| | 1.1.1-alpha.4 | [PR#2281](https://github.com/bbc/psammead/pull/2281) Change allowfullscreen attribute value to valid value `allowfullscreen` | | ||
| | 1.1.1-alpha.3 | [PR#2284](https://github.com/bbc/psammead/pull/2284) Add an AV audio skin story | | ||
| | 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 | | ||
| | 1.0.1-alpha.4 | [PR#1960](https://github.com/bbc/psammead/pull/1960) Change <React.Fragment> to <> | | ||
| | 1.0.1-alpha.3 | [PR#1961](https://github.com/bbc/psammead/pull/1961) Talos - Bump Dependencies | | ||
| | 1.0.1-alpha.2 | [PR#1942](https://github.com/bbc/psammead/pull/1942) Talos - Bump React to 16.9.0 | | ||
| | 1.0.1-alpha.1 | [PR#1920](https://github.com/bbc/psammead/pull/1920) Use @testing-library-react | | ||
| | 1.0.1-alpha.0 | [PR#1842](https://github.com/bbc/psammead/pull/1842) Fix fullscreen for Firefox, Internet Explorer and Safari | | ||
| | 1.0.0-alpha.0 | [PR#1831](https://github.com/bbc/psammead/pull/1831) Create `psammead-media-player` component | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import React from 'react'; | ||
| import { string, number } from 'prop-types'; | ||
| import Helmet from 'react-helmet'; | ||
| import { AmpImg } from '@bbc/psammead-image'; | ||
|
|
||
|
|
@@ -13,7 +13,15 @@ const AmpHead = () => ( | |
| </Helmet> | ||
| ); | ||
|
|
||
| const AmpMediaPlayer = ({ | ||
| src, | ||
| placeholderSrc, | ||
| placeholderSrcset, | ||
| title, | ||
| alt, | ||
| height, | ||
| width, | ||
| }) => { | ||
| return ( | ||
| <> | ||
| <AmpHead /> | ||
|
|
@@ -30,6 +38,9 @@ const AmpMediaPlayer = ({ src, placeholderSrc, placeholderSrcset, title }) => { | |
| src={placeholderSrc} | ||
| srcset={placeholderSrcset} | ||
| placeholder | ||
| alt={alt} | ||
| height={height} | ||
| width={width} | ||
| /> | ||
| </amp-iframe> | ||
| </> | ||
|
|
@@ -41,9 +52,15 @@ AmpMediaPlayer.propTypes = { | |
| placeholderSrc: string.isRequired, | ||
| placeholderSrcset: string, | ||
| title: string.isRequired, | ||
| alt: string, | ||
|
There was a problem hiding this comment. Also, the There was a problem hiding this comment. Added the alt, width and height as optional props because the |
||
| height: number, | ||
| width: number, | ||
| }; | ||
| AmpMediaPlayer.defaultProps = { | ||
| placeholderSrcset: null, | ||
| alt: 'Image alt', | ||
| height: 640, | ||
| width: 450, | ||
|
There was a problem hiding this comment. Provided random default values as I was not sure where to get the specific height and width There was a problem hiding this comment. Let's have There was a problem hiding this comment. When height and width are null, the error changes to There was a problem hiding this comment. I think the width and height should be required since they are in the There was a problem hiding this comment. Thanks for catching this. I think Given the width and height are only used before the AMP runtime fetches the image, can we compute values matching the aspect ratio based on If it doesn't look straightforward I'm happy if you want to create a separate issue for the articles pod to implement that and we live with the warnings for now. |
||
| }; | ||
|
|
||
| export default AmpMediaPlayer; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a new
altprop, it can be hardcoded to""on L41 for consistency with #2476