Add support for <amp-facebook-page> #13261

nainar · 2018-02-04T23:02:11Z

Most of the template is copied from similar elements like <amp-facebpook-{like,comment}>

Note: Currently the file test-amp-facebook doesn't have tests for <amp-facebpook-{like,comment}> so I didn't add any for the new , let me know if I should change that.

cvializ

Hey thanks! Just a few comments and questions.

Also could you please update your PR description to include the issue number for the bug that this resolve, along with any extra description? Consider writing details like your general approach, what you based your code on, or any areas to pay closer attention to. That can help reviewers do a better job and provide context.

cvializ · 2018-02-05T21:15:59Z

extensions/amp-facebook-page/0.1/amp-facebook-page.js

+    this.iframe_ = null;
+
+    /** @private {string} */
+    this.dataLocale_ = element.getAttribute('data-locale');


You should assign this.dataLocale_ as the result of a ternary or || expression instead of an if, so that you can annotate this.dataLocale_ as @const for more type safety

I've made the change here. However, I have written the same code in other files: amp-facebook{,-like,-comments}.js.

Do you mind if I make the change in a separate PR? Or would you like that done here. Happy either way.

Ah cool, a separate PR would be ok

cvializ · 2018-02-05T21:20:30Z

extensions/amp-facebook-page/amp-facebook-page.md

+limitations under the License.
+-->
+
+# <a name="amp-facebook-like"></a> `amp-facebook-like`


Replace this with amp-facebook-page

cvializ · 2018-02-05T21:20:40Z

extensions/amp-facebook-page/amp-facebook-page.md

@@ -0,0 +1,93 @@
+<!---
+Copyright 2017 The AMP HTML Authors. All Rights Reserved.


Done across all files

cvializ · 2018-02-05T21:20:54Z

extensions/amp-facebook-page/amp-facebook-page.md

+    layout="fixed"
+    data-hide-cover="true"
+    data-href="https://www.facebook.com/testesmegadivertidos/">
+</amp-facebook-like>


amp-facebook-page

cvializ · 2018-02-05T21:22:09Z

extensions/amp-facebook-page/validator-amp-facebook-page.protoascii

+}
+tags: {  # <amp-facebook-page>
+  html_format: AMP
+  tag_name: "AMP-FACEBOOK-page"


nit: should be all-caps AMP-FACEBOOK-PAGE

cvializ · 2018-02-05T21:24:36Z

extensions/amp-facebook-page/amp-facebook-page.md

+
+Attribute hides cover photo in the header. `false` (cover photo displayed) by default.
+
+##### data-face-pile (optional)


Should this be data-show-face-pile to match 3p/facebook.js?

cvializ · 2018-02-05T21:27:59Z

3p/facebook.js

  container.setAttribute('data-numposts', data.numposts || 10);
  container.setAttribute('data-colorscheme', data.colorscheme || 'light');
  container.setAttribute('data-width', '100%');
+  container.setAttribute('data-height', '100%');


Is this line change intentional? It looks like it affects amp-facebook-comments where this PR is for the amp-facebook-page

Nope. That was unintentional. Didn't clean it up.

honeybadgerdontcare

replace 2017 with 2018 throughout files.

nainar

Hi @cvializ I have made the changes you asked to the files and to the PR description.

PTAL?

Thanks for reviewing!

nainar · 2018-02-06T00:54:10Z

3p/facebook.js

  container.setAttribute('data-numposts', data.numposts || 10);
  container.setAttribute('data-colorscheme', data.colorscheme || 'light');
  container.setAttribute('data-width', '100%');
+  container.setAttribute('data-height', '100%');


Nope. That was unintentional. Didn't clean it up.

nainar · 2018-02-06T00:56:03Z

extensions/amp-facebook-page/amp-facebook-page.md

@@ -0,0 +1,93 @@
+<!---
+Copyright 2017 The AMP HTML Authors. All Rights Reserved.


Done across all files

nainar · 2018-02-06T00:56:42Z

extensions/amp-facebook-page/amp-facebook-page.md

+limitations under the License.
+-->
+
+# <a name="amp-facebook-like"></a> `amp-facebook-like`


nainar · 2018-02-06T00:56:52Z

extensions/amp-facebook-page/amp-facebook-page.md

+    layout="fixed"
+    data-hide-cover="true"
+    data-href="https://www.facebook.com/testesmegadivertidos/">
+</amp-facebook-like>


nainar · 2018-02-06T00:56:59Z

extensions/amp-facebook-page/amp-facebook-page.md

+
+Attribute hides cover photo in the header. `false` (cover photo displayed) by default.
+
+##### data-face-pile (optional)


nainar · 2018-02-06T00:57:13Z

extensions/amp-facebook-page/validator-amp-facebook-page.protoascii

+}
+tags: {  # <amp-facebook-page>
+  html_format: AMP
+  tag_name: "AMP-FACEBOOK-page"


nainar · 2018-02-06T01:01:36Z

extensions/amp-facebook-page/0.1/amp-facebook-page.js

+    this.iframe_ = null;
+
+    /** @private {string} */
+    this.dataLocale_ = element.getAttribute('data-locale');


I've made the change here. However, I have written the same code in other files: amp-facebook{,-like,-comments}.js.

Do you mind if I make the change in a separate PR? Or would you like that done here. Happy either way.

cvializ

Awesome! I'm happy with the code, but it looks like you'll need to make some small changes to get the gulp lint to pass.

cvializ · 2018-02-06T16:56:32Z

extensions/amp-facebook-page/0.1/amp-facebook-page.js

+    this.iframe_ = null;
+
+    /** @private {string} */
+    this.dataLocale_ = element.getAttribute('data-locale');


Ah cool, a separate PR would be ok

cvializ · 2018-02-06T16:58:43Z

extensions/amp-facebook-page/0.1/amp-facebook-page.js

+ */
+
+
+import {getIframe, preloadBootstrap} from '../../../src/3p-frame';


Looks like there's a linter error in Travis: https://travis-ci.org/ampproject/amphtml/jobs/337825128 You'll need to sort your import lines.

aghassemi · 2018-02-06T18:10:49Z

Thanks @nainar! and @cvializ for the review. Everything looks great to me.

nainar · 2018-02-07T00:59:08Z

Hi @cvializ - thanks so much for the review! I have cleaned up the lint and validator issue.

Please merge the request?

* Revision bump for #13261 - Add support for `<amp-facebook-page>` * Add media query parsing class to CSS parser. This isn't wired up to the validator currently, but will be in a later CL. * Use the recommended closure dependency management for JavaScript libraries. * Add AMP4EMAIL rules. * Remove stray console.log statments. * Small tweaks to #13406 * Revision bump for #13381

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

* Revision bump for ampproject#13261 - Add support for `<amp-facebook-page>` * Add media query parsing class to CSS parser. This isn't wired up to the validator currently, but will be in a later CL. * Use the recommended closure dependency management for JavaScript libraries. * Add AMP4EMAIL rules. * Remove stray console.log statments. * Small tweaks to ampproject#13406 * Revision bump for ampproject#13381

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

* Revision bump for ampproject#13261 - Add support for `<amp-facebook-page>` * Add media query parsing class to CSS parser. This isn't wired up to the validator currently, but will be in a later CL. * Use the recommended closure dependency management for JavaScript libraries. * Add AMP4EMAIL rules. * Remove stray console.log statments. * Small tweaks to ampproject#13406 * Revision bump for ampproject#13381

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

* Revision bump for ampproject#13261 - Add support for `<amp-facebook-page>` * Add media query parsing class to CSS parser. This isn't wired up to the validator currently, but will be in a later CL. * Use the recommended closure dependency management for JavaScript libraries. * Add AMP4EMAIL rules. * Remove stray console.log statments. * Small tweaks to ampproject#13406 * Revision bump for ampproject#13381

googlebot added the cla: yes label Feb 4, 2018

Remove spurious files

ed4d827

nainar force-pushed the amp_facebook_page branch from 597bc52 to ed4d827 Compare February 4, 2018 23:05

aghassemi requested review from aghassemi and cvializ February 5, 2018 18:35

cvializ suggested changes Feb 5, 2018

View reviewed changes

honeybadgerdontcare reviewed Feb 5, 2018

View reviewed changes

nainar added 2 commits February 6, 2018 12:12

cvializ@ suggested changes

94b7856

Merge upstream

1409eff

nainar commented Feb 6, 2018

View reviewed changes

cvializ approved these changes Feb 6, 2018

View reviewed changes

Fixed Travis CI failures

32d2e9d

nainar force-pushed the amp_facebook_page branch from c66a1c8 to 32d2e9d Compare February 6, 2018 23:24

cvializ merged commit 9673d3f into ampproject:master Feb 7, 2018

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

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

aghassemi mentioned this pull request Feb 22, 2018

Status Update 2018-02-19 #13549

Closed

protonate pushed a commit to protonate/amphtml that referenced this pull request Feb 26, 2018

Add support for <amp-facebook-page> (ampproject#13261)

8db118b

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

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

Add support for <amp-facebook-page> (ampproject#13261)

4c07087

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

protonate pushed a commit to protonate/amphtml that referenced this pull request Mar 15, 2018

Add support for <amp-facebook-page> (ampproject#13261)

8b6daa5

* Remove spurious files * cvializ@ suggested changes * Fixed Travis CI failures

aghassemi mentioned this pull request Oct 15, 2018

FR: Facebook "page reviews" and "events" components #18459

Closed

		@@ -0,0 +1,93 @@
		<!---
		Copyright 2017 The AMP HTML Authors. All Rights Reserved.


		Attribute hides cover photo in the header. `false` (cover photo displayed) by default.

		##### data-face-pile (optional)

		*/


		import {getIframe, preloadBootstrap} from '../../../src/3p-frame';

Add support for <amp-facebook-page> #13261

Add support for <amp-facebook-page> #13261

Uh oh!

Conversation

nainar commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvializ left a comment

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!

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

Uh oh!

nainar left a comment

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!

cvializ left a comment

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 6, 2018

Uh oh!

nainar commented Feb 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nainar commented Feb 4, 2018 •

edited

Loading