Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new ImagePlaceholderAmp psammead component #8914

Merged
merged 35 commits into from
Apr 7, 2021

Conversation

HarveyPeachey
Copy link
Contributor

@HarveyPeachey HarveyPeachey commented Feb 24, 2021

Resolves #8811

Overall change:
Uses new ImagePlaceholderAmp component

Code changes:

  • Updates ImageWithPlaceholder unit tests
  • Implements ImagePlaceholderAmp intoImageWithPlaceholder unit tests
  • Adds a storybook story
  • Updates snapshot tests

  • I have assigned myself to this PR and the corresponding issues
  • I have added the cross-team label to this PR if it requires visibility across World Service teams
  • I have assigned this PR to the Simorgh project

Testing:

  • Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive)
  • This PR requires manual testing

@HarveyPeachey HarveyPeachey changed the title Add new ImagePlaceholderAmp psammead compone Add new ImagePlaceholderAmp psammead component Feb 24, 2021
@HarveyPeachey HarveyPeachey self-assigned this Feb 24, 2021
@HarveyPeachey HarveyPeachey added the cross-team For visibility for both World Service teams (Engage & Media) label Feb 24, 2021
@HarveyPeachey
Copy link
Contributor Author

HarveyPeachey commented Mar 3, 2021

When bumping psammead-image-placeholder to 3.1.1 and integrating the new amp component, the PR now fails on a few existing canonical integration and e2e tests due to them not being able to find generated classnames. Those being div[class*='LoadingImageWrapper'] in the integration tests for OnDemandTV and MediaAssetPage with the asset /pashto/bbc_pashto_tv/tv_programmes/w13xttn4 being an example, and div[class*="ImagePlaceholder"], div[class*="StyledVideoContainer"] article e2e tests with /news/articles/cn7k01xp8kxo being one of the assets tested.

The classes are basically rendering without the emotion styled suffix at the end, this can be shown by running yarn run e2e:test:interactive and running the article tests and visiting the page fired up by the server http://localhost:7080/news/articles/cn7k01xp8kxo.

However when running the dev server with yarn run dev and visiting the assets being tested the suffix is showing in the class name. I'm not entirely sure how to proceed from here. The only thing that's really different is the bumping of the psammead packages to pull in the changes for image placeholder. The logic should be the same. It seems that the styled emotion classnames aren't appending for some reason on the bumped psammead components.

Copy link
Contributor Author

@HarveyPeachey HarveyPeachey left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned that this has added an extra 20 seconds to run the unit tests on my local machine against the main branch. Would be nice if people could check this on their machines as well.

I think this is due to the image placeholder component adding 6 amp fallback/placeholder images for each image on the page and snapshots like frontpage with loads of images where each one has an extra 6 images added to it takes longer for jest to make it's comparisons. The snapshot's don't reflect the implementation, as each amp image is fetched and rendered based on screen size, meaning each image will have one fallback and one placeholder amp image not 6.

What are peoples thoughts on this? We could just go for the quick and easy fix here: #8811 (comment)

@andrewscfc
Copy link
Contributor

I'm a bit concerned that this has added an extra 20 seconds to run the unit tests on my local machine against the main branch. Would be nice if people could check this on their machines as well.

I think this is due to the image placeholder component adding 6 amp fallback/placeholder images for each image on the page and snapshots like frontpage with loads of images where each one has an extra 6 images added to it takes longer for jest to make it's comparisons. The snapshot's don't reflect the implementation, as each amp image is fetched and rendered based on screen size, meaning each image will have one fallback and one placeholder amp image not 6.

What are peoples thoughts on this? We could just go for the quick and easy fix here: #8811 (comment)

Just quickly thinking about this, I'd say the problem is us using snapshots to capture whole pages for tests. The fact we have an issue with our tests shouldn't influence the fix we choose imo. I'd suggest investigating alternatives to the affected tests and very seriously considering removing some as I don't think they add a lot of value.

Copy link
Contributor

@andrewscfc andrewscfc left a comment

Choose a reason for hiding this comment

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

As discussed imo adding 20 seconds to the tests is not critical but we should follow up straightaway with a PR to remove/replace the full page snapshot tests that are affected by this change.

@@ -42,6 +45,10 @@ const ImageWithPlaceholder = ({
<Image onLoad={() => setIsLoaded(true)} {...imageProps} />
);

const AmpImgWrapper = styled.div`
position: relative;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this css needed for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Raise psammead PR as discussed to move this into psammead after merge

@LilyL0u
Copy link
Contributor

LilyL0u commented Apr 1, 2021

@HarveyPeachey In the original issue the example for this is on live. Do you have a local asset with which I can check if this has been fixed? Or a test asset to check it if we put it on test first?

@paruchurisilpa
Copy link
Contributor

paruchurisilpa commented Apr 6, 2021

Tested using localhost:7080/news/uk-55808266.amp by running a live build and the transparent images do not show the BBC blocks placeholder behind. Attached is the screenshot.

LGTM..
Screenshot 2021-04-06 at 16 54 04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cross-team For visibility for both World Service teams (Engage & Media)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PS AMP] Transparent images show the BBC blocks placeholder behind
6 participants