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

AMP validation for a single scenario #441

Closed
sareh opened this issue Aug 6, 2018 · 4 comments
Closed

AMP validation for a single scenario #441

sareh opened this issue Aug 6, 2018 · 4 comments
Labels
AMP Work related to AMP

Comments

@sareh
Copy link
Contributor

sareh commented Aug 6, 2018

Is your feature request related to a problem? Please describe.
After an AMP route has been created (#440) we want to be able to run a validation cli tool against a single page to see if it passes

Here is a page that explains what the validation tool does: https://www.ampproject.org/docs/fundamentals/validate

Describe the solution you'd like
To have an npm script in the package.json file that runs the amphtml-validator cli tool against a single article, e.g. /article/amp/scenario-01. The output should be displayed in the console. This validation does not need to be run on CI at this point. This will be handled in a future issue.

https://www.ampproject.org/docs/fundamentals/validate#command-line-tool

If there are any errors listed in the output, a separate issue should be opened to create an AMP-compliant component & container that addresses those points.

Describe alternatives you've considered
Since this is the recommended way to validate AMP pages, other options were not considered.

@sareh sareh added this to the Application Setup milestone Aug 6, 2018
@sareh sareh changed the title AMP validation against AMP validation for a single article Aug 6, 2018
@sareh sareh changed the title AMP validation for a single article AMP validation for a single scenario Aug 6, 2018
@sareh sareh added the blocked This issue should not be worked on until another internal issue is completed - see desc for details label Aug 6, 2018
@chris-hinds
Copy link
Contributor

Is the idea here to just make sure one does not break the AMP validation when making local changes? If so then this will be fine. We have been using the AMP validator for our current AMP setup since the initial launch and have not experienced any issues.

@sareh
Copy link
Contributor Author

sareh commented Aug 6, 2018

The aim in this specific Issue is to make the AMP validation output visible.
If run against a scenario with an image it will show errors, since we aren't using <amp-img> yet.
The aim is here to use the AMP validate in a way to have Test-Driven-Development of a sort - first there will be errors, then we will create Issues to address those errors (e.g. create an Image component that is AMP-compatible) then that'll make the amp-validate script no longer error.
Once we have addressed all the errors, we can then put this amp-validate as part of the CI process so that any future changes will fail the build unless addressed.

@chris-hinds
Copy link
Contributor

Ahhh I see, makes perfect sense to me in that case. :)

@jtart jtart added AMP Work related to AMP and removed blocked This issue should not be worked on until another internal issue is completed - see desc for details labels Aug 9, 2018
@sareh
Copy link
Contributor Author

sareh commented Sep 4, 2018

Resolved as fixed, in #492.

@sareh sareh closed this as completed Sep 4, 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

4 participants