Skip to content

📖<amp-next-page> documentation and validation#26841

Merged
wassgha merged 20 commits intoampproject:masterfrom
wassgha:amp-next-page-v2-docs
Feb 28, 2020
Merged

📖<amp-next-page> documentation and validation#26841
wassgha merged 20 commits intoampproject:masterfrom
wassgha:amp-next-page-v2-docs

Conversation

@wassgha
Copy link
Copy Markdown
Contributor

@wassgha wassgha commented Feb 18, 2020

Changes

  • Adds documentation and migration guide for amp-next-page version 1.0
  • Implements validation rules (although currently added on top of the 0.1 rules since there is no way to specify version-dependent validation rules so far).
  • Adds warnings to 0.1 when using features from 1.0
  • Adds validation tests
  • Implements a max-pages attribute that limits the number of recommendations shown to the user and adds tests for it
  • Implements analytics events rolled over from 0.1
  • Fixes a logic bug with the deep-parsing attribute to allow it to be disabled

To-do

  • Figure out a way to allow arbitrary content in validation (see <footer> validation error)
  • Add validation tests for next-page-hide and next-page-replace
  • Better example page

@amp-owners-bot amp-owners-bot bot requested a review from nhant01 February 18, 2020 06:59
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Feb 18, 2020 •
edited
Loading

Hey @ampproject/wg-caching, these files were changed:

extensions/amp-next-page/0.1/test/validator-amp-next-page-adsense.out
extensions/amp-next-page/0.1/test/validator-amp-next-page.out
extensions/amp-next-page/1.0/test/validator-amp-next-page-hide.html
extensions/amp-next-page/1.0/test/validator-amp-next-page-hide.out
extensions/amp-next-page/1.0/test/validator-amp-next-page-inline.html
extensions/amp-next-page/1.0/test/validator-amp-next-page-inline.out
extensions/amp-next-page/1.0/test/validator-amp-next-page-placement.html
extensions/amp-next-page/1.0/test/validator-amp-next-page-placement.out
extensions/amp-next-page/1.0/test/validator-amp-next-page-remote.html
extensions/amp-next-page/1.0/test/validator-amp-next-page-remote.out
extensions/amp-next-page/1.0/test/validator-amp-next-page-templates.html
extensions/amp-next-page/1.0/test/validator-amp-next-page-templates.out
extensions/amp-next-page/1.0/test/validator-amp-next-page.html
extensions/amp-next-page/1.0/test/validator-amp-next-page.out
extensions/amp-next-page/validator-amp-next-page.protoascii
validator/validator-main.protoascii

@wassgha wassgha requested review from Gregable and alanorozco and removed request for nhant01 February 18, 2020 07:23
CrystalOnScript
CrystalOnScript requested changes Feb 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

Please update the md file to follow the format outlined here: https://github.com/ampproject/amphtml/blob/master/build-system/tasks/extension-generator/extension-doc.template.md

Such improves would include updating "Behavior" to "Usage", migrating attributes out of tables, and moving analytics information under the Analytics header.

Please also include any aria attributes and what their behavior does under the Accessibility header, CSS selectors that can be used under the Styling header. A good example is the updated amp-accordion documentation:
https://amp.dev/documentation/components/amp-accordion/?format=websites

Thanks for your help :)

honeybadgerdontcare
honeybadgerdontcare previously requested changes Feb 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

Initial review of validation rules done.

Please add comments to your validator test files indicating if an example is valid or invalid and if invalid why.

E.g.
<!-- Valid: Correct use of amp-next-page with next-page-hidden -->
<!-- Invalid: Missing amp-next-page extension -->

CrystalOnScript
CrystalOnScript reviewed Feb 20, 2020
View reviewed changes
CrystalOnScript
CrystalOnScript requested changes Feb 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

GH is getting a little wacky on me - submitting these change requests before they're lost

CrystalOnScript
CrystalOnScript requested changes Feb 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

These comments finish this round of reviews. Thanks :)

CrystalOnScript
CrystalOnScript reviewed Feb 25, 2020
View reviewed changes
extensions/amp-next-page/amp-next-page.md Outdated
Comment on lines +190 to +191
To enable for an infinite loading experience without the need for a server-side configuration (remote-loading), `amp-next-page` automatically enables `deep-parsing` (see Attributes), which allows it to parse more suggestions by recursively looking at inline configurations inside `<amp-next-page>` tags on the loaded documents. To disable this behavior, set `deep-parsing` to `false`.

Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript Feb 25, 2020

Choose a reason for hiding this comment

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

@wassgha I still don't think these questions are answered.

CrystalOnScript
CrystalOnScript requested changes Feb 25, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

Thanks for the update, its looking a lot better!

I caught some structural changes that were lost. I also have a lot of open questions that are not answered in the document.

CrystalOnScript
CrystalOnScript requested changes Feb 25, 2020
View reviewed changes
wassgha
wassgha commented Feb 26, 2020
View reviewed changes
CrystalOnScript
CrystalOnScript requested changes Feb 27, 2020
View reviewed changes
CrystalOnScript
CrystalOnScript reviewed Feb 27, 2020
View reviewed changes
CrystalOnScript
CrystalOnScript reviewed Feb 27, 2020
View reviewed changes
CrystalOnScript
CrystalOnScript approved these changes Feb 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@CrystalOnScript CrystalOnScript left a comment

Choose a reason for hiding this comment

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

I added some last minute nits/comments. But overall, looks great - thanks for taking the time to clean it up and clarify!

@wassgha wassgha merged commit d3325ef into ampproject:master Feb 28, 2020
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Mar 2, 2020
@robinvanopstal
Merge branch 'master' into component-intro-tables
a0d419f 
* master:
  Launch `amp-next-page` v2 & clean up experiment (ampproject#27035)
  ✨Implement Display Locking on amp-accordion (ampproject#25867)
  📖<amp-next-page> documentation and validation (ampproject#26841)
  Improve visibility event tests (ampproject#26847)
  amp-geo: Fall back to API for country (ampproject#26407)
  ✨ Add customization options to <amp-story-quiz> (ampproject#26714)
  navigation: Minor test improvements (ampproject#26926)
  ♻️ Alias video.fullscreen action for symmetry with interface names (ampproject#27017)
  ✨ Implements `amp-analytics` support for `amp-next-page` (ampproject#26451)
  ✨ amp-video-iframe integration: global jwplayer() singleton by default (ampproject#26969)
  Fix unit tests broken by ampproject#26687 (ampproject#27000)
  Filter redirect info from emails (ampproject#27012)
  📖 Make amp-bind doc valid, fix amp-form stories filter (ampproject#27003)
  url-replacements: Minor test improvement (ampproject#26930)
  viewport: Minor test improvement (ampproject#26931)
  amp-consent fullscreen warning (ampproject#26467)
  dep-check: USE CAPS FOR IMPORTANCE (ampproject#26993)
  fix img url (ampproject#26987)

# Conflicts:
#	extensions/amp-next-page/amp-next-page.md
Gregable pushed a commit that referenced this pull request Mar 3, 2020
@amaltas
Validator rollup (#27067)
6ad078d 
* cl/297284764 n/a

* cl/298442636 Revision bump for #26841

* cl/298451814 Revision bump for #26294

* fix merge duplication

Co-authored-by: Amaltas <amaltas@amaltas.org>
@robinvanopstal robinvanopstal mentioned this pull request Mar 11, 2020
Closed
This was referenced Apr 22, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CrystalOnScript CrystalOnScript CrystalOnScript approved these changes

@Gregable Gregable Gregable approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

@honeybadgerdontcare honeybadgerdontcare Awaiting requested review from honeybadgerdontcare

Assignees

No one assigned

Labels

cla: yes PR use: In Beta / Experimental PR use: In Stable WG: caching

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wassgha @Gregable @honeybadgerdontcare @CrystalOnScript @googlebot @ampprojectbot