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

Move AMP iframe and Psammead components to MediaLoader #11994

Merged
merged 67 commits into from
Oct 4, 2024

Conversation

amoore108
Copy link
Contributor

@amoore108 amoore108 commented Sep 19, 2024

Overall changes

  • Moves amp-iframe to MediaLoader for AMP media rendering
  • Moves PlayButton, Message, NoScript and Guidance components out of Psammead and into MediaLoader and uplifts them to TS
  • Rejigs the way we style landscape and portrait media, moving the bulk of the layout to the figure element. This ensures that all content within the figure element flows with the aspect-ratio that is set
  • Updates snapshots where necessary
  • Removes now old AVPlayer and MediaPlayer components, as well as Psammead components no longer used

Testing

  1. Visit MediaArticlePage http://localhost:7080/hausa/articles/cdx6ddxw755o.amp?renderer_env=live
  2. Confirm the gray BBC placeholder loads before showing a 404/500 window where the media player should be (this is expected as its running locally and attempting to hit a Test endpoint) - You can get around this by appending ?renderer_env=<environment> to the end of the ampIframeUrl variable:
  3. Disable JS
  4. Confirm NoJS messaging appears
  5. Repeat for ArticlePage: http://localhost:7080/sport/cricket/articles/cvg37wdd7geo.amp?renderer_env=live
  6. Check Storybook story renders https://amp-iframe-externalembed-constructor--5d28eb3fe163f6002046d6fa.chromatic.com/?path=/story/components-medialoader--portrait

Helpful Links

Add Links to useful resources related to this PR if applicable.

Coding Standards

Repository use guidelines

@amoore108 amoore108 self-assigned this Sep 19, 2024
Comment on lines 255 to 257
styles.figure(embedded),
setAspectRatio && [
orientation === 'portrait' && styles.portraitFigure(embedded),
Copy link
Contributor Author

@amoore108 amoore108 Sep 27, 2024

Choose a reason for hiding this comment

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

We need to pass in embedded into the stylesheets because AMP will iframe and ultimately render this component. So some of the styles should only apply to the outer figure outside of the iframe and not the inner figure inside the iframe, and vice versa 🫠

@amoore108 amoore108 marked this pull request as ready for review September 30, 2024 16:20
@amoore108 amoore108 merged commit 556481e into latest Oct 4, 2024
11 checks passed
@amoore108 amoore108 deleted the amp-iframe-externalembed-constructor branch October 4, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants