AMP Social Share Extension #1856

paul-matthews · 2016-02-09T01:59:19Z

Created the AMP Social Share Extension as per Issue #1201

One extension that represents one social share button
Supports:
- twitter
- facebook
- pinterest
- linkedin
- gplus
- email
Included Tests
Includes changes to the gulp build process to include postcss-import
Includes 3p licensed content from http://codepen.io/ruandre/pen/howFi

erwinmombay · 2016-02-09T16:21:54Z

extensions/amp-social-share/0.1/amp-social-share.js

@@ -0,0 +1,148 @@
+/**
+ * Copyright 2015 The AMP HTML Authors. All Rights Reserved.


2016 here and any other new file

erwinmombay · 2016-02-10T01:19:14Z

third_party/optimized-svg-icons/styles.css

@@ -0,0 +1,50 @@
+/*


did we make any changes to this file?

Yep, I couldn't reference the contents of the file without making changes.

I extracted the svgs, and wrapped them with css classes.

we're going to probably need to add a LICENSE file in this folder as well as a README.amp that contains any modification. see https://github.com/ampproject/amphtml/blob/master/third_party/mustache/README.amp.

you might want to commit the original code (through its own standalone PR) and then through a another PR (or the same PR but different commit) do the modification, so we can see a clear delta of what has changed.

/cc @cramforce

erwinmombay · 2016-02-10T08:38:55Z

just did quick initial pass, will look into it some more tomorrow since i haven't had a chance to read the original issue yet (qoals/constraints etc).

erwinmombay · 2016-02-11T09:24:35Z

examples/amp-social-share.html

+      background-color: blue;
+    }
+  </style>
+  <style>body {opacity: 0}</style><noscript><style>body {opacity: 1}</style></noscript>


lets switch this to the new boilerplate. see https://github.com/ampproject/amphtml/blob/master/docs/create_page.md sample

erwinmombay · 2016-02-12T01:50:25Z

extensions/amp-social-share/0.1/amp-social-share.js

+    }
+    if (link === null) {
+      link = this.getWin().document.createElement(LINK_TAG);
+      link.innerHTML = this.typeConfig_['text'];


if its only text, lets use textContent here

I tried switching to textContext but it results in a collapsed anchor for the cases where no text is necessary. In other words, if i use a it'll display it as text, if I input a space it'll collapse the anchor. The most common use for this text is set it blank.

The only other option I have is to not make the content extensible. However this means that all social share providers (read: Twitter, Facebook etc) could only use images.

I'm happy to share gists of what I've tried if you think there might be another option.

Yes, this should definitely be textContent here. Few options:

You can research white-space CSS rules and how to modify them for your desired needs.

You can set a to display: block and provide it with width/height to fill the available space. In general this is a good idea since what you do is a block. However, that'd mean that you can't then host a inside of span. Although, I don't yet understand what span does and why it's needed.

You can use JS character equivalent to  . I want to say it's \u00A0, but not 100%.

I'd like to understand the markup and its intent a little better before advising the best approach. But I do lean toward display:block on a and possible no span at all.

cramforce · 2016-03-04T03:47:41Z

extensions/amp-social-share/0.1/amp-social-share.js

+      if ('maxlength' in paramConf) {
+        const maxlength = paramConf['maxlength'];
+        AMP.assert(!paramValue || paramValue.length < maxlength,
+          param + ' cannot exceed ' + maxlength + '. %s', this.element);


nit: indent 4

cramforce · 2016-03-10T01:14:55Z

extensions/amp-social-share/0.1/amp-social-share.js

+
+    /** @private @const {number} */
+    this.width_ = getLengthNumeral(this.element.getAttribute('width'))
+      || DEFAULT_WIDTH;


nit: indent 4

Addressing, but there should be a lint for this stuff :-p

cramforce · 2016-03-10T01:19:55Z

Looks great. Please squash commits and we can ship this!

cramforce · 2016-03-10T01:40:44Z

LGTM

cramforce · 2016-03-10T01:41:13Z

extensions/amp-social-share/0.1/amp-social-share.js

+      let paramValue = this.config_[param] || this.getDefaultValue_(
+          param, this.config_);
+      AMP.assert(!paramConf['required'] || paramValue !== null,
+        param + ' is a required attribute for ' + this.type_ + '. %s',


nit: 4 space indent

* Adding postcss-import to the project * Adding amp-social-share extension

AMP Social Share Extension

escobar022 · 2016-03-11T17:24:36Z

Does anyone know how long this takes to be hosted on the cdn?

erwinmombay · 2016-03-11T17:27:28Z

@escobar022 we just released this in our opt-in canary (turned on through cdn.ampproject.org/experiments.html dev-channel) https://github.com/ampproject/amphtml/releases/tag/1457636119213 , we will turn it on for 1% of pages come monday if we didnt see any problems. Then this extension should be fully live by end of day next thursday if we've deemed the version to be stable.

corysimmons · 2016-03-17T15:38:25Z

https://amp-partners.appspot.com/social demo looks good but the Twitter logo isn't retina friendly (it isn't svg whereas the others are), and it doesn't look like Facebook is displaying properly at all.

Also is there a reason these have border-radius? It's not built in is it? Perhaps we shouldn't encourage additional styles (to disable pre-existing styles) where possible?

dvoytenko · 2016-03-17T17:28:17Z

@corysimmons Would you mind opening an issue with this info?

corysimmons · 2016-03-18T04:57:56Z

Done #2628

erwinmombay reviewed Feb 9, 2016
View reviewed changes

erwinmombay added the NEEDS REVIEW label Feb 10, 2016

erwinmombay reviewed Feb 10, 2016
View reviewed changes

erwinmombay reviewed Feb 11, 2016
View reviewed changes

paul-matthews force-pushed the amp-social-share branch from 196c731 to c0a1c34 Compare February 11, 2016 23:28

erwinmombay reviewed Feb 12, 2016
View reviewed changes

cramforce reviewed Mar 4, 2016
View reviewed changes

cramforce mentioned this pull request Mar 4, 2016

Intent to implement: Viafoura embed #2442

Closed

paul-matthews force-pushed the amp-social-share branch 2 times, most recently from ab6577b to 3901312 Compare March 10, 2016 01:03

cramforce reviewed Mar 10, 2016
View reviewed changes

paul-matthews force-pushed the amp-social-share branch from 3901312 to 3db8802 Compare March 10, 2016 01:38

cramforce added LGTM and removed NEEDS REVIEW labels Mar 10, 2016

cramforce reviewed Mar 10, 2016
View reviewed changes

Implementing AMP Social Share Extension

519b3e8

* Adding postcss-import to the project * Adding amp-social-share extension

paul-matthews force-pushed the amp-social-share branch from 3db8802 to 519b3e8 Compare March 10, 2016 01:43

cramforce added a commit that referenced this pull request Mar 10, 2016

Merge pull request #1856 from paul-matthews/amp-social-share

b80230b

AMP Social Share Extension

cramforce merged commit b80230b into ampproject:master Mar 10, 2016

paul-matthews mentioned this pull request Mar 10, 2016

amp-social-share template components for social sharing buttons #1201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AMP Social Share Extension #1856

AMP Social Share Extension #1856

paul-matthews commented Feb 9, 2016

erwinmombay Feb 9, 2016

erwinmombay Feb 10, 2016

paul-matthews Feb 10, 2016

erwinmombay Feb 10, 2016

erwinmombay commented Feb 10, 2016

erwinmombay Feb 11, 2016

erwinmombay Feb 12, 2016

paul-matthews Feb 12, 2016

dvoytenko Feb 12, 2016

cramforce Mar 4, 2016

paul-matthews Mar 10, 2016

cramforce Mar 10, 2016

paul-matthews Mar 10, 2016

cramforce commented Mar 10, 2016

cramforce commented Mar 10, 2016

cramforce Mar 10, 2016

escobar022 commented Mar 11, 2016

erwinmombay commented Mar 11, 2016

corysimmons commented Mar 17, 2016

dvoytenko commented Mar 17, 2016

corysimmons commented Mar 18, 2016

		@@ -0,0 +1,148 @@
		/**
		* Copyright 2015 The AMP HTML Authors. All Rights Reserved.

AMP Social Share Extension #1856

AMP Social Share Extension #1856

Conversation

paul-matthews commented Feb 9, 2016

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

erwinmombay commented Feb 10, 2016

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

cramforce commented Mar 10, 2016

cramforce commented Mar 10, 2016

Choose a reason for hiding this comment

escobar022 commented Mar 11, 2016

erwinmombay commented Mar 11, 2016

corysimmons commented Mar 17, 2016

dvoytenko commented Mar 17, 2016

corysimmons commented Mar 18, 2016