Fix prop type warnings #2516

Bopchy · 2019-10-28T17:37:14Z

Resolves #2497

Overall change: Add notes to psammead-media-player stories. Fix prop type warnings.

Code changes:

Add notes to psammead-media-player stories.
Fix prop type warnings.

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

Bopchy · 2019-10-29T13:29:14Z

This PR hasn't fixed - Warning: Failed prop type: The prop widthis marked as required inAmpImg, but its value is undefined. and Warning: Failed prop type: The propheightis marked as required inAmpImg, but its value is undefined.

Considered making height & width props optional in psammead-image (see #2523), however this will require AmpImg in psammead-image/src/index.amp.jsx to always be used within a container with specified height or width so that AMP can use the correct ratio for scaling the images

UPDATE - Closed #2523 and instead provided a default value for the height and width

… into 2497-fix-prop-type-warnings

ghost

Still seeing warnings while running `test:unit`

I was running an out of date version of the branch.

ghost

Looks great.

ghost · 2019-10-29T16:19:41Z

packages/components/psammead-media-player/src/Amp/index.jsx

@@ -41,9 +52,15 @@ AmpMediaPlayer.propTypes = {
  placeholderSrc: string.isRequired,
  placeholderSrcset: string,
  title: string.isRequired,
+  alt: string,


Should this be a required prop?

Also, the width and height are required in the AmpImg component.

Added the alt, width and height as optional props because the <AmpMediaPlayer /> seems to work without them - as it's getting the height and width from the StyledVideoContainer and StyledAudioContainer

ghost · 2019-10-29T16:20:23Z

packages/components/psammead-media-player/src/Amp/index.jsx

+  height: 640,
+  width: 450,


Where are we getting this values from?

Provided random default values as I was not sure where to get the specific height and width

Let's have null as the default values. The same thing we have on psammead-image component

When height and width are null, the error changes to Warning: Failed prop type: The prop height is marked as required in AmpImg, but its value is null.. React doesn't allow required props to be null. Found this issue on that - facebook/react#3163

I think the width and height should be required since they are in the AmpImg component. Can you double-check this with @RayNjeri, please?

Thanks for catching this. I think height and width should be required props here and not given arbitrary default values.

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 portrait, see how we do https://github.com/bbc/psammead/blob/7b7d4eb86022e7edbfa37bd1a43c4d1633f6f967/packages/components/psammead-media-player/src/index.jsx#L11 and pass these in?

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.

… into 2497-fix-prop-type-warnings

packages/components/psammead-media-player/src/index.stories.jsx

jamesdonoh · 2019-11-01T12:40:47Z

packages/components/psammead-media-player/src/Amp/index.jsx

+  placeholderSrc,
+  placeholderSrcset,
+  title,
+  alt,


I don't think we need a new alt prop, it can be hardcoded to "" on L41 for consistency with #2476

jamesdonoh · 2019-11-01T12:50:04Z

packages/components/psammead-media-player/src/Amp/index.jsx

+  height: 640,
+  width: 450,


Thanks for catching this. I think height and width should be required props here and not given arbitrary default values.

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 portrait, see how we do https://github.com/bbc/psammead/blob/7b7d4eb86022e7edbfa37bd1a43c4d1633f6f967/packages/components/psammead-media-player/src/index.jsx#L11 and pass these in?

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.

…ype-warnings

Bopchy · 2019-11-01T15:27:43Z

Added a height and width of 16 or 9 depending on if portrait == true or not

…ype-warnings

jamesdonoh

Thanks for the changes.

Ruth Ogendi added 5 commits October 28, 2019 16:15

Add notes to psammead-media-player stories.

8c99439

Update package.json, package-lock.json, CHANGELOG.md.

d8153db

Add alt, height & width as props.

8f5ff8d

Make alt a prop.

46fdcdb

Update snapshots.

b4528b5

Bopchy self-assigned this Oct 28, 2019

Ruth Ogendi added 2 commits October 28, 2019 19:37

Update PR number & link.

f198da6

Remove extra white spaces.

3a7308c

Bopchy added the ws-home Tasks for the WS Home Team label Oct 29, 2019

Bopchy marked this pull request as ready for review October 29, 2019 10:20

Bopchy requested review from jamesbhobbs, dr3, j-pendlebury, jroebu14 and sareh as code owners October 29, 2019 10:20

Bopchy mentioned this pull request Oct 29, 2019

Make height and width optional #2523

Closed

3 tasks

EinsteinNjoroge and others added 4 commits October 29, 2019 16:53

Merge branch 'latest' into 2497-fix-prop-type-warnings

221cc9c

Add default height and width values.

7a42260

Update snapshot.

d35b560

Merge branch '2497-fix-prop-type-warnings' of github.com:bbc/psammead…

04fc8c5

… into 2497-fix-prop-type-warnings

ghost previously requested changes Oct 29, 2019

View reviewed changes

ghost approved these changes Oct 29, 2019

View reviewed changes

ghost reviewed Oct 29, 2019

View reviewed changes

Ruth Ogendi added 2 commits October 31, 2019 18:14

Merge branch '2497-fix-prop-type-warnings' of github.com:bbc/psammead…

9d76a32

… into 2497-fix-prop-type-warnings

Merge branch 'latest' into 2497-fix-prop-type-warnings

0f4378a

Ruth Ogendi added 2 commits October 31, 2019 18:38

Update version number.

2a8f24b

Merge branch '2497-fix-prop-type-warnings' of github.com:bbc/psammead…

0f89154

… into 2497-fix-prop-type-warnings

EinsteinNjoroge reviewed Nov 1, 2019

View reviewed changes

packages/components/psammead-media-player/src/index.stories.jsx Outdated Show resolved Hide resolved

EinsteinNjoroge reviewed Nov 1, 2019

View reviewed changes

packages/components/psammead-media-player/src/index.stories.jsx Outdated Show resolved Hide resolved

jamesdonoh suggested changes Nov 1, 2019

View reviewed changes

Ruth Ogendi added 5 commits November 1, 2019 14:59

Make height & width required props. Remove alt.

7150525

Combine canoncial and amp stories.

8c796db

Merge branch 'latest' of github.com:bbc/psammead into 2497-fix-prop-t…

f583767

…ype-warnings

Add heigth and width.

62d8c6b

Update snapshots.

f57e957

Ruth Ogendi added 4 commits November 1, 2019 17:47

Add height and width to tests.

ecc3833

Update snapshot.

78d31d9

Update README.

09b15f3

Merge branch 'latest' of github.com:bbc/psammead into 2497-fix-prop-t…

444c08e

…ype-warnings

RayNjeri approved these changes Nov 4, 2019

View reviewed changes

Bopchy requested a review from jamesdonoh November 4, 2019 10:22

EinsteinNjoroge approved these changes Nov 4, 2019

View reviewed changes

jamesdonoh approved these changes Nov 4, 2019

View reviewed changes

sareh approved these changes Nov 4, 2019

View reviewed changes

sareh merged commit c7fe403 into latest Nov 4, 2019

sareh deleted the 2497-fix-prop-type-warnings branch November 4, 2019 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix prop type warnings #2516

Fix prop type warnings #2516

Bopchy commented Oct 28, 2019 •

edited

Loading

Bopchy commented Oct 29, 2019 •

edited

Loading

ghost left a comment

ghost left a comment

ghost Oct 29, 2019

DenisHdz Oct 29, 2019

Bopchy Oct 30, 2019

ghost Oct 29, 2019

Bopchy Oct 30, 2019

ghost Oct 30, 2019

Bopchy Oct 30, 2019 •

edited

Loading

DenisHdz Nov 1, 2019

jamesdonoh Nov 1, 2019

jamesdonoh Nov 1, 2019

jamesdonoh Nov 1, 2019

Bopchy commented Nov 1, 2019 •

edited

Loading

jamesdonoh left a comment

Fix prop type warnings #2516

Fix prop type warnings #2516

Conversation

Bopchy commented Oct 28, 2019 • edited Loading

Bopchy commented Oct 29, 2019 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bopchy Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bopchy commented Nov 1, 2019 • edited Loading

jamesdonoh left a comment

Choose a reason for hiding this comment

Bopchy commented Oct 28, 2019 •

edited

Loading

Bopchy commented Oct 29, 2019 •

edited

Loading

Bopchy Oct 30, 2019 •

edited

Loading

Bopchy commented Nov 1, 2019 •

edited

Loading