Initial commit of amp-access-scroll #12955

kushal · 2018-01-22T22:25:11Z

Adds an access vendor named scroll
If user is authenticated, adds UI element to page

aghassemi · 2018-01-24T20:45:17Z

@dvoytenko Anyone else familiar with access you recommend to review this?
/cc @rudygalfi

dvoytenko · 2018-01-25T04:45:20Z

dvoytenko · 2018-01-25T05:24:09Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+      buildUrl: accessService.buildUrl.bind(accessService),
+      collectUrlVars: accessService.collectUrlVars_.bind(accessService),
+    });
+    this.element_ = null;


dvoytenko · 2018-01-25T05:24:35Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+/**
+ * @extends {HTMLElement}
+ */
+class ScrollElement extends HTMLElement {


Is this now possible on all browsers? I feel like I've run into some compatibility issues before.

i think i was confused about what you wanted here, put something in for now

dvoytenko · 2018-01-25T05:25:51Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+class ScrollElement extends HTMLElement {
+  constructor(ampdoc) {
+    super();
+    installStylesForDoc(ampdoc, CSS, () => {}, false, TAG);


Could you pls check if installStylesForDoc protects from inserting stylesheet twice?

yup, it does

dvoytenko · 2018-01-25T05:26:03Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+    super();
+    installStylesForDoc(ampdoc, CSS, () => {}, false, TAG);
+
+    this.ampdoc = ampdoc;


jsdoc here and below.

dvoytenko · 2018-01-25T05:26:58Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+    this.placeholder_.classList.add('amp-access-scroll-placeholder');
+    this.wrapper_.appendChild(this.placeholder_);
+
+    const img = document.createElement('amp-img');


Also, totally fine to use just normal <img>, especially for a logo. This kind of logo might just be better to do via CSS background, but up to you.

just changed to img for now

dvoytenko · 2018-01-25T05:28:40Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+    this.iframe_.setAttribute('allowtransparency', 'true');
+    this.iframe_.setAttribute('title', 'Scroll');
+    this.iframe_.setAttribute('width', '100%');
+    this.iframe_.setAttribute('height', '100%');


Is it possible to set sandbox for this iframe?

should be fine

dvoytenko · 2018-01-25T05:29:24Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+  }
+
+  show() {
+    Services.accessServiceForDoc(this.ampdoc)


accessService is already passed in the constructor. Can you save it there and use it here?

dvoytenko · 2018-01-25T05:30:28Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+            this.wrapper_.removeChild(this.placeholder_);
+          };
+          this.iframe_.setAttribute('src',
+              'https://connect.scroll.com/amp/scrollbar?readerId=' + readerId);


Let's do encodeURIComponent(readerId). Should be websafe, but this way it's for sure.

dvoytenko · 2018-01-25T05:31:48Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+  }
+}
+
+customElements.define('amp-access-scroll-elt',


Do we have to define it in CE? That's a bit rife with incompatibilties. I think it'd be just safer all around to just have ScrollElement a wrapper/holder class. Is there a strong reason to register it in CE?

kushal

Thanks!

kushal · 2018-01-26T00:27:28Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+      buildUrl: accessService.buildUrl.bind(accessService),
+      collectUrlVars: accessService.collectUrlVars_.bind(accessService),
+    });
+    this.element_ = null;


kushal · 2018-01-26T00:27:32Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+/**
+ * @extends {HTMLElement}
+ */
+class ScrollElement extends HTMLElement {


i think i was confused about what you wanted here, put something in for now

kushal · 2018-01-26T00:27:36Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+class ScrollElement extends HTMLElement {
+  constructor(ampdoc) {
+    super();
+    installStylesForDoc(ampdoc, CSS, () => {}, false, TAG);


yup, it does

kushal · 2018-01-26T00:27:37Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+    super();
+    installStylesForDoc(ampdoc, CSS, () => {}, false, TAG);
+
+    this.ampdoc = ampdoc;


kushal · 2018-01-26T00:27:40Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+    this.placeholder_.classList.add('amp-access-scroll-placeholder');
+    this.wrapper_.appendChild(this.placeholder_);
+
+    const img = document.createElement('amp-img');


just changed to img for now

kushal · 2018-01-26T00:27:43Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+    this.iframe_.setAttribute('allowtransparency', 'true');
+    this.iframe_.setAttribute('title', 'Scroll');
+    this.iframe_.setAttribute('width', '100%');
+    this.iframe_.setAttribute('height', '100%');


should be fine

kushal · 2018-01-26T00:27:45Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+            this.wrapper_.removeChild(this.placeholder_);
+          };
+          this.iframe_.setAttribute('src',
+              'https://connect.scroll.com/amp/scrollbar?readerId=' + readerId);


kushal · 2018-01-26T00:27:47Z

extensions/amp-access-scroll/0.1/scroll-impl.js

+  }
+}
+
+customElements.define('amp-access-scroll-elt',


kushal · 2018-01-26T00:31:05Z

By the way, I think the answer is no, but is there any way for our extension to also automagically provision our amp-analytics URLs? Also, should I add an OWNERS file?

- Adds an access vendor named scroll - If user is authenticated, adds UI element to page

* Revision bump for #13036 * Revision bump for #13073 * Add DEDUPE_ON_MINIFY flags to license tags so that release process can reduce the number of identical duplicate licenses in the minified validator. * Add requires_extension to AttrSpec. * JSDoc updates. * Generated validator javascript improvements. * Add comment to ValidationError hinting at how to render. * Revision bump for #12955 * Add new error types for future CSS validation. * Revision bump for #12798 * Fix a typo. * Allow animation-timing-function for keyframes * Fix typo

* Initial commit of amp-access-scroll - Adds an access vendor named scroll - If user is authenticated, adds UI element to page * Review fixes * Fix tests * Add OWNERS file

* Revision bump for ampproject#13036 * Revision bump for ampproject#13073 * Add DEDUPE_ON_MINIFY flags to license tags so that release process can reduce the number of identical duplicate licenses in the minified validator. * Add requires_extension to AttrSpec. * JSDoc updates. * Generated validator javascript improvements. * Add comment to ValidationError hinting at how to render. * Revision bump for ampproject#12955 * Add new error types for future CSS validation. * Revision bump for ampproject#12798 * Fix a typo. * Allow animation-timing-function for keyframes * Fix typo

* Initial commit of amp-access-scroll - Adds an access vendor named scroll - If user is authenticated, adds UI element to page * Review fixes * Fix tests * Add OWNERS file

* Revision bump for ampproject#13036 * Revision bump for ampproject#13073 * Add DEDUPE_ON_MINIFY flags to license tags so that release process can reduce the number of identical duplicate licenses in the minified validator. * Add requires_extension to AttrSpec. * JSDoc updates. * Generated validator javascript improvements. * Add comment to ValidationError hinting at how to render. * Revision bump for ampproject#12955 * Add new error types for future CSS validation. * Revision bump for ampproject#12798 * Fix a typo. * Allow animation-timing-function for keyframes * Fix typo

* Initial commit of amp-access-scroll - Adds an access vendor named scroll - If user is authenticated, adds UI element to page * Review fixes * Fix tests * Add OWNERS file

* Revision bump for ampproject#13036 * Revision bump for ampproject#13073 * Add DEDUPE_ON_MINIFY flags to license tags so that release process can reduce the number of identical duplicate licenses in the minified validator. * Add requires_extension to AttrSpec. * JSDoc updates. * Generated validator javascript improvements. * Add comment to ValidationError hinting at how to render. * Revision bump for ampproject#12955 * Add new error types for future CSS validation. * Revision bump for ampproject#12798 * Fix a typo. * Allow animation-timing-function for keyframes * Fix typo

jpettitt · 2020-02-07T19:30:33Z

@kushal please create a .md file with doc for your extension

googlebot added the cla: yes label Jan 22, 2018

dvoytenko self-requested a review January 25, 2018 04:45

dvoytenko suggested changes Jan 25, 2018

View reviewed changes

kushal commented Jan 26, 2018

View reviewed changes

dvoytenko approved these changes Feb 2, 2018

View reviewed changes

kushal added 3 commits February 1, 2018 22:57

Initial commit of amp-access-scroll

f6fd8ce

- Adds an access vendor named scroll - If user is authenticated, adds UI element to page

Review fixes

11e8c01

Fix tests

0cfeff6

kushal force-pushed the scrollextension2 branch from 4966bff to 0cfeff6 Compare February 2, 2018 04:02

Add OWNERS file

1111e7a

dvoytenko merged commit 274b63c into ampproject:master Feb 5, 2018

rsimha mentioned this pull request Feb 5, 2018

Fix import ordering in amp-access-scroll #13265

Merged

rsimha self-assigned this Feb 5, 2018

Gregable pushed a commit that referenced this pull request Feb 7, 2018

Revision bump for #12955

729665b

ampprojectbot added the PR use: In Stable label Feb 14, 2018

kushal deleted the scrollextension2 branch March 28, 2018 22:04

rsimha removed their assignment Feb 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial commit of amp-access-scroll #12955

Initial commit of amp-access-scroll #12955

kushal commented Jan 22, 2018

aghassemi commented Jan 24, 2018

dvoytenko commented Jan 25, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

dvoytenko Jan 25, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

dvoytenko Jan 25, 2018

kushal Jan 26, 2018

kushal left a comment

kushal Jan 26, 2018

kushal Jan 26, 2018

kushal Jan 26, 2018

kushal Jan 26, 2018

kushal Jan 26, 2018

kushal Jan 26, 2018

kushal Jan 26, 2018

kushal Jan 26, 2018

kushal commented Jan 26, 2018 •

edited

Loading

jpettitt commented Feb 7, 2020

Initial commit of amp-access-scroll #12955

Initial commit of amp-access-scroll #12955

Conversation

kushal commented Jan 22, 2018

aghassemi commented Jan 24, 2018

dvoytenko commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kushal commented Jan 26, 2018 • edited Loading

jpettitt commented Feb 7, 2020

kushal commented Jan 26, 2018 •

edited

Loading