Add amp script if amp html attr present #718
Conversation
| ); | ||
| } | ||
|
|
||
| if (!isAmpPayload()) { |
There was a problem hiding this comment.
Keeping this as a seperate if statement, not an else, as we are likely to have additional requirements to conditionally inject the client JS or application.
…bbc/simorgh into add-amp-script-if-amp-html-attr-present
| let ampScript; | ||
| let serialisedData; | ||
|
|
||
| if (isAmpPayload(helmet.htmlAttributes)) { |
There was a problem hiding this comment.
I've added the logic to handle the injection of the amp <script> and the removal of other <script>'s here in Document as it is currently an isolated use case. In the future it is likely that we will need to inject different scripts for different use cases, at this point it may be more beneficial to abstract the logic into a Scripts component similarly to how we handle MetaData within an isolated component.
There was a problem hiding this comment.
We should just handle injection of the AMP script in the MetaData component. This component already has access to a boolean of amp so the logic is simple to add.
Would just need to extract scripts out of react-helmet and inject it into the document.
…bbc/simorgh into add-amp-script-if-amp-html-attr-present
| let ampScript; | ||
| let serialisedData; | ||
|
|
||
| if (isAmpPayload(helmet.htmlAttributes)) { |
There was a problem hiding this comment.
We should just handle injection of the AMP script in the MetaData component. This component already has access to a boolean of amp so the logic is simple to add.
Would just need to extract scripts out of react-helmet and inject it into the document.
| </head> | ||
| <body> | ||
| {/* eslint-disable react/no-danger */ | ||
| /* disabling the rule that bans the use of dangerouslySetInnerHTML until a more appropriate implementation can be implemented */} |
There was a problem hiding this comment.
Can this be added back to the root element?
| ); | ||
| } | ||
|
|
||
| if (!isAmpPayload(helmet.htmlAttributes)) { |
There was a problem hiding this comment.
Rather than do this logic here, data returns an amp boolean, which would be cleaner to use.
We could extract all of the script creation to that level, including the creation of of the data script, async AMP script, and deferred bundle scripts. It can all go in a single script array, which can be pased to the Document for it to use.
|
Code Climate has analyzed commit 09cb4d4 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (90% is the threshold). This pull request will bring the total coverage in the repository to 96.2% (0.0% change). View more on Code Climate. |
cypress/integration/ampSpec.js
Outdated
| @@ -17,6 +17,12 @@ describe('AMP Tests', () => { | |||
| getElement('html').should('have.attr', 'amp'); | |||
| }); | |||
|
|
|||
| it('should load the AMP framework', () => { | |||
| // .eq(1) gets the amp <script> as the first loaded is a Cypress <script> | |||
| const ampScript = getElement('html script').eq(1); | |||
There was a problem hiding this comment.
It would be good to update this cypress test to be more specific - to check this script is in the head of the page. e.g. with this selector: 'head script'
|
Code Climate has analyzed commit edcb50e and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (90% is the threshold). This pull request will bring the total coverage in the repository to 96.4% (0.0% change). View more on Code Climate. |
Resolves #678
Checks if
helmet.htmlAttributesin Document/index.jsx containsampif so injects the amp script (<script async src="https://cdn.ampproject.org/v0.js" />) and does not render other custom js, including the client side application