Minimal (experimental) document-level infinite-scroll extension #13184

emarchiori · 2018-01-31T18:45:08Z

amp-document-recommendations

Initial minimal extension for feedback and for incremental implementation. Settled on temporary name, avoiding infinite-scroll as UX currently has recommendations in between to create separation.

This PL does:

Load documents using the MultidocManager after the current doc
Show recommendations that use A2A linking between documents, to provide a natural break
Export MultidocManager so that it can be used by this extension

This PL does not:

Implement everything in the I2I
Include tests yet (waiting for feedback on the approach)
Manage any edge-cases (e.g. fixed position elements) or error-cases
Have a final UI

Related-to #12945

cc/ @aghassemi @peterjosling

googlebot · 2018-01-31T18:45:12Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-02-01T17:09:45Z

CLAs look good, thanks!

aghassemi · 2018-02-02T19:13:24Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.css

+ * limitations under the License.
+ */
+
+i-amp-document-recommendations {}


There is no tag called i-amp-document-recommendations right? I think this can be removed.

aghassemi · 2018-02-02T19:14:29Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.css

+
+i-amp-document-recommendations {}
+
+.i-amp-document-recommendations {


naming convention is i-amphtml-component-name for private CSS classes.

aghassemi · 2018-02-02T19:14:43Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.css

+i-amp-document-recommendations {}
+
+.i-amp-document-recommendations {
+  background: white;


nit: use hex

aghassemi · 2018-02-02T19:15:26Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+ */
+
+/** @const */
+const MAX_ARTICLES = 2;


nit: varibales after imports

aghassemi · 2018-02-02T19:16:01Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+  constructor(element) {
+    super(element);
+
+    this.element.classList.add('i-amp-document-recommendations');


move to buildCallback

aghassemi · 2018-02-02T19:21:08Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+    this.element.classList.add('i-amp-document-recommendations');
+
+    // TODO(emarchiori): Consider using a service instead of singleton.
+    if (this.win.CONTENT_DISCOVERY) {


I assume this is simply a flag so that the component is only rendered once? If so, just having it be a variable outside of class (e.g. at extension level) would be better for now until this becomes a window level service.

Correct. Done.

aghassemi · 2018-02-02T19:23:30Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+    return true;
+  }
+
+  appendDivision_() {


jsdocs for all methods.

aghassemi · 2018-02-02T19:27:25Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+      this.nextArticle_++;
+
+      Services.xhrFor(this.win)
+          .fetchDocument(next.ampUrl, {})


No need to pass {} since it is optional.

aghassemi · 2018-02-02T19:29:34Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+  appendArticleLinks_(from) {
+    const doc = this.win.document;
+    let article = from;
+    const viewer = Services.viewerForDoc(this.getAmpDoc());


do in constructor and keep this.viewer_

aghassemi · 2018-02-02T19:30:53Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+      const next = this.config_.recommendations[article];
+      article++;
+
+      const articleHolder = doc.createElement('div');


use button for A11Y, otherwise need to handle taxIndex and enter/space activation manually.

peterjosling · 2018-02-16T14:31:59Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+
+    this.config_ = assertConfig(configJson);
+
+    return this.mutateElement(() => {


Other components (amp-list, amp-date-picker) make XHR requests for their content from the layoutCallback - probably makes sense for this too?

aghassemi · 2018-02-20T19:37:16Z

@emarchiori @peterjosling Please ping me on this PR when ready for the final look. Thanks!

peterjosling · 2018-02-22T11:59:05Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+ * @const
+ * TODO(emarchiori): Make this a configurable parameter.
+ */
+const SEPARATPOR_RECOS = 3;


typo: SEPARATOR

peterjosling · 2018-02-22T11:59:50Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.css

+}
+
+.i-amphtml-reco-holder-article {
+  padding-top:10px;


nit: space between colon and value (for each line in this block)

peterjosling · 2018-02-22T12:00:45Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+
+  /** @override */
+  isLayoutSupported(layout) {
+    return layout == Layout.CONTAINER;


Keeping == for consistency with other extensions and rest of the code.

we had a long discussions about this in the team :) although style guides says ===, we are sticking with ==

peterjosling · 2018-02-22T15:57:29Z

examples/amp-document-recommendations.amp.html

+      {
+        "recommendations": [
+          {
+            "image": "http://localhost:8000/examples/img/hero@1x.jpg",


Remove http://localhost:8000 and just have these as absolute paths ("image": "/examples/img/hero@1x.jpg", etc.) The host/port can be overridden.

This does not handle any edge cases or error cases, and does all the loading ahead.

…ing behaviors.

Having trouble running check-types locally becase of ampproject#11574.

…rong place.

…dations

aghassemi

few nits and one request for checking that experiment is enabled in buildCallback.

aghassemi · 2018-02-22T22:02:09Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+  }
+
+  /** @override */
+  buildCallback() {


needs a

if (!isExperimentOn(this.getWin(), TAG)) { user.warn('Experiment %s is not turned on.', TAG); return; }

Used an assert, for consistency with other experiments.

aghassemi · 2018-02-26T22:08:19Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.css

+  background: #FFFFFF;
+}
+
+.amp-document-recommendations-division {


nit: keep public css class on top of the CSS file

aghassemi · 2018-02-26T22:10:33Z

extensions/amp-document-recommendations/0.1/amp-document-recommendations.js

+
+  /** @override */
+  isLayoutSupported(layout) {
+    return layout == Layout.CONTAINER;


we had a long discussions about this in the team :) although style guides says ===, we are sticking with ==

aghassemi · 2018-02-27T23:30:27Z

@emarchiori @peterjosling merged.

honeybadgerdontcare · 2018-02-28T00:15:29Z

extensions/amp-document-recommendations/validator-amp-document-recommendations.protoascii

+tags: {  # <amp-document-recommendations>
+  html_format: AMP
+  tag_name: "AMP-DOCUMENT-RECOMMENDATIONS"
+  mandatory_parent: "BODY"


Is there a reason this needs to have parent of body and why it must be the last child of body?

This is a restriction that we might want to remove or relax it in the future, but forcing those two conditions reduces potentially unexpected behaviors (e.g. no footer on the first article as there are others in between, but the following article having a footer in the middle as it does not load other articles recursively).

Adding support to hide things like footer in all but the last article and headers in all but the first can probably led to the relaxation of this in followup changes.

Any particular concerns with this restrictions?

Yes, it means no other component can also be last in the body. It makes this component have some significance among all components. @aghassemi thoughts?

Spoke with @aghassemi to clarify intent. Thank you.

* Revision bump for #13184 * Revision bump for #13480, #13665 * Revision bump for #13752 * Revision bump for #13773 * Removing AMP-YOUTUBE tag from AMP4EMAIL validator spec. * A unit test that ensures no extensions are accidentally added to EXPERIMENTAL amp html format. * Implement amp-story-auto-ads validation rules. * Revision bump for #13846 and #13816 * Revision bump for #13883

…pproject#13184)

* Revision bump for ampproject#13184 * Revision bump for ampproject#13480, ampproject#13665 * Revision bump for ampproject#13752 * Revision bump for ampproject#13773 * Removing AMP-YOUTUBE tag from AMP4EMAIL validator spec. * A unit test that ensures no extensions are accidentally added to EXPERIMENTAL amp html format. * Implement amp-story-auto-ads validation rules. * Revision bump for ampproject#13846 and ampproject#13816 * Revision bump for ampproject#13883

googlebot added the cla: no label Jan 31, 2018

aghassemi requested review from aghassemi and dvoytenko January 31, 2018 23:44

emarchiori force-pushed the document-recommendations branch from 26b1b67 to ed4aee0 Compare February 1, 2018 17:09

googlebot added cla: yes and removed cla: no labels Feb 1, 2018

aghassemi reviewed Feb 2, 2018

View reviewed changes

peterjosling reviewed Feb 16, 2018

View reviewed changes

peterjosling reviewed Feb 22, 2018

View reviewed changes

emarchiori added 8 commits February 26, 2018 09:05

Intial code set up for the amp-document-recommendations extension.

638ec67

This does not handle any edge cases or error cases, and does all the loading ahead.

Fix all lint errors.

eb4f9cb

Address all suggestions from @aghassemi.

8b5acd2

Starts making XHR requests on layout callback and adds TODOs for miss…

afbf2c6

…ing behaviors.

Fix missing import.

b587ed3

Address comments and fix compiler errors.

d978085

Make config_ and multidocManager_ nullable.

61b0ad3

Having trouble running check-types locally becase of ampproject#11574.

Update manual example, and remove validator tests which were in the w…

b4c2f57

…rong place.

emarchiori force-pushed the document-recommendations branch from 634e553 to b4c2f57 Compare February 26, 2018 09:07

Merge remote-tracking branch 'upstream/master' into document-recommen…

c17b044

…dations

aghassemi reviewed Feb 26, 2018

View reviewed changes

Properly check that experiment is enabled and Fix css rule order.

d678407

aghassemi changed the title ~~Minimal document-level infinite-scroll extension~~ Minimal (experimental) document-level infinite-scroll extension Feb 27, 2018

aghassemi approved these changes Feb 27, 2018

View reviewed changes

aghassemi merged commit 4d3dd44 into ampproject:master Feb 27, 2018

honeybadgerdontcare reviewed Feb 28, 2018

View reviewed changes

emarchiori deleted the document-recommendations branch February 28, 2018 13:49

RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018

Minimal (in-development) document-level infinite-scroll extension (am…

593b8a4

…pproject#13184)


		i-amp-document-recommendations {}

		.i-amp-document-recommendations {


		this.config_ = assertConfig(configJson);

		return this.mutateElement(() => {

Minimal (experimental) document-level infinite-scroll extension #13184

Minimal (experimental) document-level infinite-scroll extension #13184

Uh oh!

Conversation

emarchiori commented Jan 31, 2018

amp-document-recommendations

This PL does:

This PL does not:

Uh oh!

googlebot commented Jan 31, 2018

Uh oh!

googlebot commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi commented Feb 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!