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

Create PlatformContext and Change isAmp to be a constant platform #927

Closed
1 task done
ChrisBAshton opened this issue Nov 12, 2018 · 1 comment
Closed
1 task done
Assignees
Labels
AMP Work related to AMP

Comments

@ChrisBAshton
Copy link
Contributor

ChrisBAshton commented Nov 12, 2018

Is your feature request related to a problem? Please describe.
Arising from the meeting in #884: we've identified that one day we may have to support more than just 'canonical' and 'AMP'.

To prevent lock-in to binary platform delivery, we should phase out our use of the isAmp boolean and instead refer to a platform string which may have the value 'canonical', 'AMP' or something else in the future.

Notes from Jim:

So at the moment the assumption is that FIA and Apple News stay with Syndication
However we may want to render a syndicatable HTML version of the article - this is something Bogdan and I have discussed a little
so it does make sense to consider a world where we are rendering more than two variants

Describe the solution you'd like
Create the PlatformContext, and use PlatformContex.Provider in the Article container & PlatformContainer.Consumer in the Image container. A POC can be seen here: #878.

Rather than passing isAmp as props, pass platform via the context. Almost everywhere in our code where we refer to isAmp, we should make a change.

That said, in containers which need AMP logic, there's nothing stopping us from doing isAmp = platform === 'amp' and then referring to isAmp in the rest of the file.

Describe alternatives you've considered
The alternative is passing the platform as a prop down nested components - quickly gets messy.

Another alternative is some centralised state (eg Redux), but this isn't necessary at this time.

The alternative is to keep things the way they are - but could become a maintenance nightmare if that 3rd platform becomes a requirement in the future! Making the change to platform means we'd have to make changes to a minimal number of files.

Testing notes
Unit tests should be sufficient.

@ChrisBAshton ChrisBAshton added Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. AMP Work related to AMP alpha-2 labels Nov 12, 2018
@ChrisBAshton ChrisBAshton added this to the 00. Application Setup milestone Nov 12, 2018
@ChrisBAshton
Copy link
Contributor Author

To be merged with #928.

@dr3 dr3 changed the title Change isAmp to be a constant platform Create PlatformContext and Change isAmp to be a constant platform Nov 15, 2018
@BogdanDogaru BogdanDogaru reopened this Nov 15, 2018
@BogdanDogaru BogdanDogaru removed the Refinement Needed This is ready for refinement. It shouldn't be worked on until it has been refined by Dev & Test. label Nov 15, 2018
@bcmn bcmn self-assigned this Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMP Work related to AMP
Projects
None yet
Development

No branches or pull requests

3 participants