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

AV-Embeds Next.js routing #11804

Open
wants to merge 34 commits into
base: latest
Choose a base branch
from
Open

AV-Embeds Next.js routing #11804

wants to merge 34 commits into from

Conversation

amoore108
Copy link
Contributor

@amoore108 amoore108 commented Jul 28, 2024

Part of https://jira.dev.bbc.co.uk/browse/WSTEAM1-1191

Overall changes

  • Adds routing to handle av-embeds routes in the Next.js app and a basic debug output
  • Moves ThemeProvider out of PageWrapper and into withContexts and _app, beside the other Context providers. This is required as the av-embeds routes do not need to render the page furniture (header/footer etc) that PageWrapper provides, but should still be able to produce a correctly styled 404/500 page if an incorrect route is provided

Code changes

  • Adds handling of the av-embeds route in the [service]/[[...]].page.tsx catch-all
  • Adds parsing logic to extract values from requested URL to allow data fetching for media content
  • Adds boilerplate for AV_EMBEDS analytics
  • Moves ThemeProvider out of PageWrapper and into withContexts for the Express app and _app for the Next.js app
  • Updates Storybook stories that need more granular ThemeProvider control

Testing

  1. Run branch locally
  2. Visit: http://localhost:7081/serbian/cyr/av-embeds/srbija-68707945
  3. Check it parses the URL and returns an object with the expected values
  4. Visit: http://localhost:7081/russian/av-embeds/features-49881797/pid/p07q3wwl
  5. Check it parses the URL and returns an object with the expected values
  6. Visit: http://localhost:7081/serbian/cyr/av-embed/srbija-50103048/pid/p07rfhrv
  7. Check it returns a 404 page (note the missing s in av-embeds in the URL

Helpful Links

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

Coding Standards

Repository use guidelines

@amoore108 amoore108 self-assigned this Jul 28, 2024
const env = getEnvironment(resolvedUrl);
const agent = certsRequired(resolvedUrl) ? await getAgent() : null;

const parsedRoute = parseAvRoute(resolvedUrl);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is by no means final. More for debugging and testing URL parsing.

Comment on lines +157 to +159
// Assumes /ws/ routes are purely for Simorgh AMP pages
// - only for testing
const isSyndicationRoute = !query.includes('ws');
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is still a draft / WIP, but wondering what is happening with AMP + Media Player? Or do we think that there might still be a need for an AMP version of the av-embed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea this is focused mainly on syndication routes for now. The ws/av-embeds routes will need to be supported for AMP media players in Article pages. The logic here does support those routes too (at least parsing out the values from the URL), so they are pretty much good to go.

@amoore108 amoore108 marked this pull request as ready for review July 31, 2024 13:16
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.

None yet

3 participants