Implementing amp-playbuzz extension (#6106) by buzzdan · Pull Request #6351 · ampproject/amphtml

buzzdan · 2016-11-27T22:23:13Z

Hi everyone,
This is our initial implementation of extension as discussed in #6106
Any feedback would be appreciated

Thanks a lot,
Dan Mordechay
Playbuzz

googlebot · 2016-11-27T22:23:16Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

buzzdan · 2016-11-28T09:29:04Z

I signed it!

jridgewell · 2016-11-28T21:03:54Z

/to @dvoytenko, feel free to reassign.

googlebot · 2016-11-29T12:04:35Z

CLAs look good, thanks!

dvoytenko · 2016-11-29T17:31:28Z

extensions/amp-playbuzz/0.1/amp-playbuzz.css

+ */
+
+.pb_feed_loading {
+        height: 320px;


Please use 2sp for indent.

Hmm. Not seeing the changes. Did you push the latest commit?

dvoytenko · 2016-11-29T17:32:06Z

extensions/amp-playbuzz/0.1/amp-playbuzz.css

+ * limitations under the License.
+ */
+
+.pb_feed_loading {


To confirm: are these styles meant to be customized by clients? Or are they strictly internal?

internal. it's the loader's styles

Ok. It's our convention to name internal classes with - prefix. But it's up to you.

dvoytenko · 2016-11-29T17:32:38Z

extensions/amp-playbuzz/0.1/amp-playbuzz.css

+}
+
+.pb_top_content_container {
+        opacity: 1;


The style above conflicts with this one.

Whoops, my bad

dvoytenko · 2016-11-29T17:33:32Z

extensions/amp-playbuzz/0.1/amp-playbuzz.css

+}
+
+.pb_feed_placeholder_container {
+        position: absolute;


Placeholders (attribute placeholder) are automatically sized 100% of the container. Or is this for some other placeholder functionality?

that's taking care of the internal arrangement of the placeholder + loading indicator

dvoytenko · 2016-11-29T17:34:59Z

extensions/amp-playbuzz/0.1/amp-playbuzz.css

+        box-sizing: border-box;
+}
+
+.pb_ie_fixer {


Please add a comment up top of what specifically is fixed in IE with this style.

actually i tried using css animation before i noticed AMP doesnt allow it
this was a fix for that in IE but since it's no longer relevant i'll remove it

AMP generally allows CSS animations and we can allow it here as well. Let's come back to it in a follow up PR.

dvoytenko · 2016-11-29T18:08:25Z

extensions/amp-playbuzz/0.1/utils.js

+    }
+  }
+  catch (e) {
+    dev().log(err, e);


There's no log() method. Can you please double check? Also, generally, it's easier to just use rethrowAsync.

dvoytenko · 2016-11-29T18:09:52Z

extensions/amp-playbuzz/0.1/utils.js

+    return {};
+  }
+
+  dev().log(err, data);


Ditto: there's no log method. Also, most of methods take tag name as the first argument. E.g. amp-playbuzz.

dvoytenko · 2016-11-29T18:11:16Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+  generateEmbedSourceUrl_() {
+    const params = {};
+    params.itemUrl = this.item_.replace('https://', '//').replace('http://', '//').split('#')[0];
+    params.relativeUrl = params.itemUrl.split('.playbuzz.com')[1];


Please use url.js APIs for this.

i was looking for something like this! nice!

dvoytenko · 2016-11-29T18:11:55Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+      `Enable ${EXPERIMENT} experiment`);
+
+    const e = this.element;
+    this.item_ = user().assert(


What are requirements for this URL? Should it be https? Please use assertHttpsUrl in that case.

no requirement for https, i just need it to be a valid playbuzz url.
a publisher can just copy his item url (http/https) and pase it into the amp-playbuzz src field and it should work.
that's the reason i'm dropping the scheme and use double slashes // for the beginning (to avoid cors issues)

dvoytenko · 2016-11-29T18:13:02Z

extensions/amp-playbuzz/0.1/utils.js

+  const embedUrl = options.itemUrl +
+      '?feed=true' +
+      '&implementation=amp' +
+      '&src=' + options.itemUrl +


Looks like you need encodeURIComponent here. Generally, you can just use serializeQueryString API from url.js to do this for you. There are a couple of places below with the same issue.

very nice util ! thanks!

dvoytenko · 2016-11-29T18:14:00Z

@buzzdan This looks great! I left a few comments. PTAL.

buzzdan · 2016-12-01T17:42:40Z

@dvoytenko Thanks for the detailed review!
see my fixes + replies

dvoytenko · 2016-12-01T23:08:50Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+
+    this.applyFillContent(iframe);
+    this.element.appendChild(iframe);
+    setStyles(this.element, {'height': '300px'});


Please remove

dvoytenko · 2016-12-01T23:09:10Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+
+    setStyles(iframe, {
+      'opacity': 0,
+      'min-height': '300px',


Please remove all height designators.

dvoytenko · 2016-12-01T23:12:28Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+      'min-height': '300px',
+      'height': '300px',
+    });
+    setStyles(this.element, {'height': '300px', 'min-height': '300px'});


It shouldn't be getting 100%, but possible if you CSS somewhere resizes it this way. For now, let's focus on user-specified height and let it do the work. We can look if we can/need do default later. In the example above, the responsive layout will automatically resize you embed to take all available width and then will set the height proportional to the width. In that particular case, given layout=responsive width=300 height=300, the height will be equal to the width. If that's not the case, please check your CSS and if you don't see the reason - let me know and I'll check it on my end.

dvoytenko · 2016-12-01T23:25:21Z

@buzzdan Ok, looks good. I think in major things, only height questions are outstanding and scrolling.

googlebot · 2016-12-05T15:23:56Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-12-05T16:24:12Z

CLAs look good, thanks!

dvoytenko · 2016-12-05T19:02:35Z

@buzzdan Hi! Saw a big push. Ready for re-review?

buzzdan · 2016-12-06T15:56:18Z

@dvoytenko so far i've dealt mostly with the height issue therefore added an overflow element (i needed to, right?), so we can review that part.

about the scroll events, they are crucial for our items as it ties in directly with the UX we are trying to provide.
by removing it we'll be providing a broken experience for our partners, which something we'd really like to avoid.
we'd be happy to present in-person the different use-cases and further discuss how we can solve this issue.

Gregable

My review is for the validator rules only.

Gregable · 2016-12-06T17:49:12Z

extensions/amp-playbuzz/0.1/validator-amp-playbuzz.protoascii

+  html_format: AMP
+  html_format: AMP4ADS
+  tag_name: "SCRIPT"
+  spec_name: "amp-playbuzz extension .js script"


For new tags, we want to add a requirement that if the .js file is present, a corresponding tag is also present, to avoid javascript being loaded which isn't used, a performance anti-pattern. This is fairly new, so most tags don't have it, but an example would be https://github.com/ampproject/amphtml/blob/master/extensions/amp-reddit/0.1/validator-amp-reddit.protoascii

Can you add here:

also_requires_tag: "amp-playbuzz"

Gregable · 2016-12-06T17:53:38Z

extensions/amp-playbuzz/0.1/validator-amp-playbuzz.protoascii

+  tag_name: "AMP-PLAYBUZZ"
+  also_requires_tag: "amp-playbuzz extension .js script"
+  attrs: {
+    name: "src"


all optional:

Do you want any constraints on this src attribute value to help folks spot errors? Specifically, you could enforce that this is a URL (see https://github.com/ampproject/amphtml/blob/master/extensions/amp-list/0.1/validator-amp-list.protoascii for an example).

Similarly, do you want to require any of the data- attributes shown in your examples? All data- attributes are allowed by default, but this means that small typos don't typically generate errors and can be subtle to catch. You can require a set here if that is your preference.

@Gregable thanks a lot! I'll look into it

dvoytenko · 2016-12-07T20:17:57Z

i've dealt mostly with the height issue therefore added an overflow element (i needed to, right?), so we can review that part.

Overflow element (at least initially) is desirable - yes. In the follow up PRs we can look into this in a bit more detail.

about the scroll events, they are crucial for our items as it ties in directly with the UX we are trying to provide.

Yes, I'd definitely like to see this. Feel free to ping me on Slack with times. In particular, I have Thur and Fri this week fairly open. My suggestion for now was to merely delay this feature for the first pull request. There are still some steps we need to take to deploy the extension and update validators, so I'm eager to get the initial merge as soon as practical; while other details can worked out next. If you really want to get scroll events on the first merge - then, yes, let's get together at the earliest convenience and take a look if we can speed their inclusion up.

dvoytenko · 2016-12-07T20:18:52Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+import {Layout, isLayoutSizeDefined} from '../../../src/layout';
+import {removeElement} from '../../../src/dom';
+import {isExperimentOn} from '../../../src/experiments';
+// import {setStyles} from '../../../src/style';


Please remove.

dvoytenko · 2016-12-07T20:20:45Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+
+  /** @override */
+  unlayoutCallback() {
+    this.unlisteners_.forEach(unlisten => unlisten());


Also add this.unlisteners_.length = 0;

dvoytenko · 2016-12-07T20:24:59Z

@buzzdan Otherwise, it looks like we are ready pending scroll events. I added couple more comments. PTAL.

dvoytenko · 2016-12-08T23:11:14Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

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


Ping on this question. I'm pretty sure you just need isLayoutSizeDefined and it should be ok with default rules.

dvoytenko · 2016-12-08T23:13:40Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+
+    return this.iframePromise_ = this.loadPromise(iframe).then(() => {
+
+      this.attemptChangeHeight(this.itemHeight_).catch(() => {/* die */});


There's no a lot of reason to call attemptChangeHeight here. Normally you only need to update height when your embed sends you a request to do so.

Are you just trying to make sure you have some minimal height here? (300px?) If yes - this code can actually reduce the size of your element in this case.

dvoytenko · 2016-12-08T23:16:20Z

@buzzdan Just a couple more comments here, mainly on the initial height. We'll chat tomorrow about scroll events. We can touch base on the height as well. Generally (pending scroll discussion), I can already merge this code and we can clear up additional questions as a follow up.

dvoytenko · 2016-12-09T18:37:24Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+      event: 'scroll',
+      windowHeight: changeEvent.height,
+      scroll: changeEvent.top,
+      offsetTop: viewport.getLayoutRect(this.element).top,


Please use this.getLayoutBox().top for offsetTop.

about the layout:
from my tests responsive & fixed-height layouts work as expected and we can support it starting now.

fixed layout would just be broken since the item cant be displayed fully

for some reason fill layout floats over the rest of the elements with position:absolute
(from the docs it seems that its supposed to catch its partent height and width)

flex-item might work with some extra adjusments but for now placeholder seems to be broken with it and the item wouldnt load. i need to look deeper into it.

@buzzdan

fixed layout would just be broken since the item cant be displayed fully

Can't displayed fully horizontally you mean? Because you set your own height vertically...

for some reason fill layout floats over the rest of the elements with position:absolute
(from the docs it seems that its supposed to catch its partent height and width)

It takes the height/width of its offset parent, i.e. a parent with position:relative or position:absolute.

flex-item might work with some extra adjusments but for now placeholder seems to be broken with it and the item wouldnt load. i need to look deeper into it.

Flex-item should work - it's a convenient mechanism of authors who like to use flexbox for layout.

dvoytenko · 2016-12-09T18:43:32Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

+    const scrollingData = {
+      event: 'scroll',
+      windowHeight: changeEvent.height,
+      scroll: changeEvent.top,


So, to repeat question here. The intersection observer use is mostly equivalent if this code was:

{ scroll: scrollTop - offsetTop, offsetTop: 0, }

The nuance here is that you don't know the exact top scroll position of the parent document and your position within it. But you DO know exactly which part of your embed is visible. Basically this emulates a situation where your embed is always at the very top of the page (offsetTop = 0).

Can you please comment on this? I'd like to get an idea if this is sufficient information for you or if you need more?

Thanks for the detailed comment, this technology is very interesting.
let me get back to you soon as we're looking into it on our side

Hey Dima, we need the scrolling events to understand where our iframe's content is relative to the top of the page, and we need this information to be up-to-date. So, understanding that sending a postMessage to the iframe on every scroll is too much, we're sending this postMessage using debounce.
Using intersection observer seems like a really cool idea, since we're getting all the data we need in the callback (and therefore don't have to query for the iframe's offset, for example), but in order to receive this event as often as we need it, we will have to provide the observer with a pretty long threshold array. In this scenario, won't it be a little inefficient?

Ok. Understood. We'll look into continuous intersection observer idea. In the meantime, I'm approving an exception to use scroll events here.

dvoytenko · 2016-12-09T18:44:31Z

extensions/amp-playbuzz/0.1/amp-playbuzz.js

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


This is still outstanding. I believe you should use isLayoutSizedDefined here since you update height later anyway.

googlebot · 2016-12-11T23:26:11Z

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.

googlebot · 2016-12-11T23:57:27Z

CLAs look good, thanks!

dvoytenko · 2016-12-12T21:41:32Z

@buzzdan @luoruize Couple small items outstanding with layout. But I'm going to go ahead and merge in the meantime. Thanks a lot for walking me through the use cases, this has been very helpful!

* Implementing amp-playbuzz extension (ampproject#6106) * Improved current implementation of amp-playbuzz * Fixed height issue * Fixed CR issues for amp-playbuzz * Change amp-playbuzz height + homemade overflow button (ampproject#6106) * Fixed origin issue * Fixed Height bug + Origin Check * Updated validation files + unlisteners array * Fixed scrolling events before overflow bug * Fixed placeholder strech height bug * Fixed scroll data + layout support + slow preconnect * Removed debouncing from OnChange to receive all events

- (#17012) `amp-access-iframe` (cleanup issue #13287) - (#25480) `amp-mega-menu` - (#25516) `amp-nested-menu` - (#6947) `amp-playbuzz` (cleanup issue #6351) - (#25170) `amp-sidebar-swipe-to-dismiss` - (#21058) `fixed-elements-in-lightbox` - (#25512) `swg-gpay-native` - (#26021) `swg-gpay-api`

- (ampproject#17012) `amp-access-iframe` (cleanup issue ampproject#13287) - (ampproject#25480) `amp-mega-menu` - (ampproject#25516) `amp-nested-menu` - (ampproject#6947) `amp-playbuzz` (cleanup issue ampproject#6351) - (ampproject#25170) `amp-sidebar-swipe-to-dismiss` - (ampproject#21058) `fixed-elements-in-lightbox` - (ampproject#25512) `swg-gpay-native` - (ampproject#26021) `swg-gpay-api`

Implementing amp-playbuzz extension (#6106)

8fd7a51

jridgewell assigned dvoytenko Nov 28, 2016

jridgewell added the 3P label Nov 28, 2016

Improved current implementation of amp-playbuzz

1f37c63

buzzdan added 2 commits November 29, 2016 18:49

Fixed height issue

cac2860

Resolved merge conflicts

47c3e16

dvoytenko added the NEEDS REVIEW label Nov 29, 2016

dvoytenko suggested changes Nov 29, 2016

View reviewed changes

Fixed CR issues for amp-playbuzz

6db67a4

Merge branch 'master' into extensions/amp-playbuzz

4ac47d8

dvoytenko reviewed Dec 1, 2016

View reviewed changes

Change amp-playbuzz height + homemade overflow button (#6106)

597633b

Fixed origin issue

f64e056

Gregable reviewed Dec 6, 2016

View reviewed changes

Fixed Height bug + Origin Check

98ba13d

dvoytenko suggested changes Dec 7, 2016

View reviewed changes

erwinmombay removed the NEEDS REVIEW label Dec 7, 2016

Updated validation files + unlisteners array

b9e19ce

dvoytenko suggested changes Dec 8, 2016

View reviewed changes

dvoytenko suggested changes Dec 9, 2016

View reviewed changes

buzzdan added 2 commits December 11, 2016 13:38

Fixed scrolling events before overflow bug

7c890d6

Fixed placeholder strech height bug

891944b

Fixed scroll data + layout support + slow preconnect

6e3731a

Removed debouncing from OnChange to receive all events

5d48696

dvoytenko approved these changes Dec 12, 2016

View reviewed changes

dvoytenko merged commit accd6eb into ampproject:master Dec 12, 2016

dvoytenko mentioned this pull request Jan 9, 2017

Launch amp-playbuzz experiment #6947

Merged

alanorozco added a commit to alanorozco/amphtml that referenced this pull request Dec 4, 2020

ampproject#6351 amp-playbuzz

e3b0fb1

alanorozco added a commit to alanorozco/amphtml that referenced this pull request Dec 4, 2020

(ampproject#6351) amp-playbuzz

0c34332

alanorozco mentioned this pull request Dec 4, 2020

🚮 Launched experiment sweep #31438

Merged


		return this.iframePromise_ = this.loadPromise(iframe).then(() => {

		this.attemptChangeHeight(this.itemHeight_).catch(() => {/* die */});

Conversation

buzzdan commented Nov 27, 2016

Uh oh!

googlebot commented Nov 27, 2016

Uh oh!

buzzdan commented Nov 28, 2016

Uh oh!

jridgewell commented Nov 28, 2016

Uh oh!

googlebot commented Nov 29, 2016

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!

dvoytenko commented Nov 29, 2016

Uh oh!

buzzdan commented Dec 1, 2016

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!

dvoytenko commented Dec 1, 2016

Uh oh!

googlebot commented Dec 5, 2016

Uh oh!

googlebot commented Dec 5, 2016

Uh oh!

dvoytenko commented Dec 5, 2016

Uh oh!

buzzdan commented Dec 6, 2016

Uh oh!

Gregable left a comment

Choose a reason for hiding this comment

Uh oh!

Gregable Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gregable Dec 6, 2016 •

edited

Loading

Gregable Dec 6, 2016 •

edited

Loading