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

I2I: Consolidated 1.0 amp-facebook #34969

Closed
caroqliu opened this issue Jun 22, 2021 · 2 comments
Closed

I2I: Consolidated 1.0 amp-facebook #34969

caroqliu opened this issue Jun 22, 2021 · 2 comments
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code WG: bento WG: components

Comments

@caroqliu
Copy link
Contributor

caroqliu commented Jun 22, 2021

Summary

This I2I proposes consolidating amp-facebook:0.1, amp-facebook-comments:0.1, amp-facebook-like:0.1, and amp-facebook-page:0.1 into a single amp-facebook:1.0 component.

Design Document

API impact

The amp-facebook component currently supports a required data-embed-as attribute with value post|video|comment. This proposal involves allowing three more embed types: comments|like|page.

- <amp-facebook-comments>
+ <amp-facebook data-embed-as="comments">

- <amp-facebook-like>
+ <amp-facebook data-embed-as="like">

- <amp-facebook-page>
+ <amp-facebook data-embed-as="page">

All other pre-existing attributes on standalone components will be supported in amp-facebook. Invalid combinations of their existing attributes will behave as follows:

  • valid AMP, which is not different from the current behavior given that we allow data-* attributes by default
  • noop if irrelevant to the embed type, which is also the status quo. Take for example amp-facebook-page, which will follow the same code path as amp-facebook[data-embed-as=page]:

    amphtml/3p/facebook.js

    Lines 145 to 160 in c194d57

    function getPageContainer(global, data) {
    const container = createContainer(global, 'page', data.href);
    container.setAttribute('data-tabs', data.tabs);
    container.setAttribute('data-hide-cover', data.hideCover);
    container.setAttribute('data-show-facepile', data.showFacepile);
    container.setAttribute('data-hide-cta', data.hideCta);
    container.setAttribute('data-small-header', data.smallHeader);
    container.setAttribute('data-adapt-container-width', true);
    const c = global.document.getElementById('c');
    // Note: The facebook embed allows a maximum width of 500px.
    // If the container's width exceeds that, the embed's width will
    // be clipped to 500px.
    container.setAttribute('data-width', c./*OK*/ offsetWidth);
    return container;
    }

The downside is that document authors will not be able to convert the import scripts for components from 0.1 to 1.0 as the only migration action required. If this is an issue, we can consider publishing valid amp-facebook-{comments,like,page} with 1.0 versions which upgrade to amp-facebook[data-embed-as]. These could also be transformed as such on the AMP cache.

Attribute matrix

The follow table outlines the attributes current supported by each component individually.

amp-facebook -comments -like -page
data-href x x x x
data-locale x x x x
title x x x x
data-allowfullscreen x
data-embed-as x
data-include-common-parent x
data-colorscheme x x
data-numposts x
data-orderby x
data-action x
data-kd_site x
data-layout x
data-ref x
data-share x
data-size x
data-hide-cover x
data-hide-cta x
data-show-facepile x
data-small-header x
data-tabs x

Behavior impact

The primary behavior change is that all embed types will now attemptChangeHeight, which impacts amp-facebook and amp-facebook-comments -- the only two components that would prior forceChangeHeight and cause CLS regardless if it is pushing content down below it, potentially disrupting an end user's engagement with content. To be clear, amp-facebook-like and amp-facebook-page have always used attemptChangeHeight and would continue to do so as <amp-facebook data-embed-as="like"> and <amp-facebook data-embed-as="page">. Examples would be provided in all use cases of how to provide an overflow element to solicit user interaction for resizing.

amp-facebook will also fail if its data-embed-as attribute is not one of the six embed types, whereas prior it would restrict it to the existing three.

Motivation

Without this change, AMP developers are forced to import up to 4 components scripts which all: preconnect to the Facebook SDK, create an iframe, and load our third-party Facebook integration code. They could get the same feature set with only one such component with negligible code additions to ensure compatibility and significant removals of redundant logic.

For publishers who only use one of these components on their document, the difference would be that this configuration object would have 15 more entries:

BaseElement['props'] = {
...FacebookBaseElement['props'],
'allowFullScreen': {attr: 'data-allowfullscreen'},
'includeCommentParent': {
attr: 'data-include-comment-parent',
type: 'boolean',
default: false,
},
'showText': {attr: 'data-show-text'},
};

Alternative Solutions

We can do nothing and launch the four components as-is. The same features are available regardless, but publishers would see some non-optimized performance when using more than one of these on a single document. Namely, this includes downloading near-identical extension binaries in all cases and also downloading near-identical pre-upgrade CSS for non-AMP (Bento) use cases.

Launch Tracker

No response

Notifications

/cc @ampproject/wg-approvers @ampproject/wg-components

@caroqliu caroqliu added the INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code label Jun 22, 2021
@caroqliu
Copy link
Contributor Author

caroqliu commented Jun 23, 2021

Resolution from design review:

  • amp-facebook:1.0 will add support for data-embed-as=post|video|comment|comments|like|page to simplify usage.
  • amp-facebook-{comments|like|page} will be exposed with 1.0 versions and registered with a consolidated amp-facebook extension binary, so upgrading versions will look as follows:
    <script async 
    -  custom-element="amp-facebook-{comments|like|page}" 
    +  custom-element="amp-facebook" 
    -  src="https://cdn.ampproject.org/v0/amp-facebook-comments-0.1.js"
    +  src="https://cdn.ampproject.org/v0/amp-facebook-1.0.js"
    ></script>
    Note that the above script will validate and enable use of <amp-facebook-comments>, <amp-facebook-like>, and <amp-facebook-page>
  • We will not be making any modifications to cache behavior to support this change.
  • We will go forward with all components moving toward an attemptChangeHeight behavior.

@caroqliu
Copy link
Contributor Author

caroqliu commented Aug 9, 2021

This is closed by #35395

@caroqliu caroqliu closed this as completed Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code WG: bento WG: components
Projects
None yet
Development

No branches or pull requests

2 participants