[POC] [WIP] AMP validation fixes #878
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
src/app/containers/Figure/index.jsx
Outdated
| @@ -15,10 +16,14 @@ const renderCaption = captionValue => | |||
| <Text text={captionValue} paragraphOverride={Caption} /> | |||
| ) : null; | |||
|
|
|||
| const FigureContainer = ({ src, alt, ratio, copyright, caption }) => ( | |||
| const FigureContainer = ({ src, alt, ratio, copyright, caption, platform }) => ( | |||
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| import { PlatformContextProvider, PlatformContextConsumer } from './index'; | ||
|
|
||
| describe('PlatformContext', () => { | ||
| const testBrandNameWithServiceContext = platform => { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
Fixes http://localhost:7080/news/articles/c9rpqy7pmypo.amp:55:14676 Custom JavaScript is not allowed. (see https://www.ampproject.org/docs/reference/spec#html-tags)
|
Code Climate has analyzed commit 1571299 and detected 5 issues on this pull request. Here's the issue category breakdown:
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.8% (0.0% change). View more on Code Climate. |
This was referenced Nov 1, 2018
2 tasks
ChrisBAshton
changed the title
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #873. This PR:
amp-boilerplateCSS (encapsulated in one commit) - fixes Fix AMP validation: boilerplate AMP CSS #886.http://localhost:7080/news/articles/c9rpqy7pmypo.amp:51:2530 The tag 'head > style[amp-boilerplate] - old variant' appears more than once in the document. (see https://github.com/ampproject/amphtml/blob/master/spec/amp-boilerplate.md)amp-imgonly).The latter is the biggest change and introduces new conventions which ought to be discussed as a team.
AMP at the component level
We need to render different HTML depending on whether we're rendering the AMP page or the canonical page. This is in order for the AMP page to validate correctly.
For now, I've decided to create an
ampdirectory inside any component which requires an AMP-specific override.Initially, I forwarded the
isAmpboolean down the rendering pipeline to the Block container, so that the component is aware of which version is needed. However, on second thoughts, anisAmpboolean is quite limited, in that we may also need to create custom modified components for other platforms in future (Facebook Instant Articles and Apple News, to name a couple). So I mapisAmpto aplatformstring, where so far we have two possible options:ampandcanonical.#873 probably requires breaking down further, as full AMP validation & integration will involve changing lots of code. In addition to rendering different HTML at the component level, for some AMP components we will need to pull in additional AMP libraries in the
<head>of the page. Plus, the Storybook integration requires some thought; currently AMP isn't pulled into the Storybook page so it is not possible to render the AMP versions of the component.AMP validation errors
So far, this PR gets our AMP errors down from:
...to:
The 'focusable' issue is encapsulated in #679 and is blocked on AMP-core, and the 'amp-custom' issue is encapsulated in #883 and is blocked on styled-components.
TODO: