✨ implement amp-embedly-card component #14819

juanlizarazo · 2018-04-24T05:31:43Z

PR that implements amp-embedly-card component (feature request #12908) using embedly cards.

juanlizarazo · 2018-04-24T05:42:15Z

@aghassemi @alanorozco New PR ready for review 😁

aghassemi · 2018-04-26T00:55:44Z

I will defer the full review to @alanorozco

On the API side: Is there a particular reason amp-embedly-key is a separate component? I suggest having a data-key attribute on the <amp-embedly-card> itself for consistency.

juanlizarazo · 2018-04-26T02:37:43Z

@aghassemi

It was a change requested on the original <amp-embedly>'s PR here.

But maybe it made more sense then as it was required. For the new <amp-embedly-card> the paid api key is optional and it just removes branding.

I'll be happy to remove the additional amp-embdely-key component! Your call @aghassemi and @alanorozco

aghassemi · 2018-04-27T01:06:37Z

@juanlizarazo I see. I don't have a strong opinion, leaving the final decision to @alanorozco on this.

alanorozco · 2018-04-24T23:06:13Z

validator/testdata/feature_tests/amp-embedly-card.out

@@ -0,0 +1 @@
+PASS


Since you're submitting validator rules, could you please follow the steps described here to generate test cases?

alanorozco · 2018-05-02T04:43:26Z

extensions/amp-embedly-card/0.1/amp-embedly-card-impl.js

+
+  /** @override */
+  layoutCallback() {
+    return getEmbedlyServiceForDoc(this.element).then(service => {


I know I originally suggested a service, but there's be a better way.

Remove the service

Simply query for

document.querySelector('amp-embedly-key').getAttribute('data-key');

The service suggestion was way overdesigned, I apologize.

I also read the other thread regarding the API. Unless you have an objection and after this change, it looks good to me!

Thanks Alan, I will be updating this PR tonight :D

aghassemi · 2018-05-30T17:09:55Z

Keeping this PR open since there is progress.

juanlizarazo · 2018-06-11T06:03:11Z

@alanorozco

Updated as requested:

Removed docservice to set/get embedly api key.
Query value attribute on element as recommended.
Added validator test cases and updated .out file.

alanorozco · 2018-06-11T16:47:28Z

extensions/amp-embedly-card/amp-embedly-card.md

+```
+
+Setting a key through the `amp-embedly-key` component is optional. If you are a paid user 
+this will remove Embedly's branding from the cards.


This tag must only be included once

alanorozco · 2018-06-11T16:48:59Z

extensions/amp-embedly-card/validator-amp-embedly-card.protoascii

+  }
+}
+
+tags: {  # <amp-embedly-key>


Add unique: true

honeybadgerdontcare · 2018-06-11T17:49:56Z

extensions/amp-embedly-card/validator-amp-embedly-card.protoascii

+  requires_extension: "amp-embedly-card"
+  attrs: {
+    name: "data-url"
+    mandatory: true


You may want to consider adding some restrictions on the url. e.g. https only. See the UrlSpec for what can be restricted. It can be set as a value_url on the tags spec, e.g.

value_url: { allowed_protocol: "https" allow_relative: false }

honeybadgerdontcare · 2018-06-11T17:52:29Z

extensions/amp-embedly-card/validator-amp-embedly-card.protoascii

+tags: {  # <amp-embedly-card>
+  html_format: AMP
+  tag_name: "AMP-EMBEDLY-CARD"
+  satisfies: "amp-embedly-card"


This isn't needed (satisfies)

honeybadgerdontcare · 2018-06-11T17:55:34Z

validator/testdata/feature_tests/amp-embedly-card.html

@@ -0,0 +1,69 @@
+<!doctype html>


This should probably be in extensions/amp-embedly-card/0.1/tests/ as validator-amp-embedly-card.html and the .out file as well.

Moved to recommended folder and renamed.

juanlizarazo

Updated with recommended changes.

alanorozco

This is awesome, really clean and thorough.

alanorozco · 2018-06-12T15:21:15Z

@bpaduch For assistance with docs.

juanlizarazo · 2018-06-12T15:47:27Z

Awesome, thanks!

@alanorozco @bpaduch @aghassemi

For the examples page in examples/amp-embedly-card.amp.html should we remove the used key in <amp-embedly-key> just comment it out and have branded cards there?

As keys are paid, that key eventually will be eventually invalid.

alanorozco · 2018-06-12T15:50:50Z

@juanlizarazo

Sure, removing the key sounds good.

ghost · 2018-06-12T16:43:11Z

extensions/amp-embedly-card/amp-embedly-card.md

+  </tr>
+  <tr>
+    <td width="40%"><strong>Required Script</strong></td>
+    <td><code>&lt;script async custom-element="amp-form" src="https://cdn.ampproject.org/v0/amp-embedly-card-0.1.js">&lt;/script></code></td>


Change "amp-form" to "amp-embedly-card"

ghost · 2018-06-12T16:43:40Z

extensions/amp-embedly-card/amp-embedly-card.md

+<table>
+  <tr>
+    <td width="40%"><strong>Description</strong></td>
+    <td>Provides you with responsive and shareable embeds to drive the reach of your websites, blog posts, and articles. from any URL using <a href="http://docs.embed.ly/docs/cards">Embedly cards</a>.</td>


remove period after articles

ghost · 2018-06-12T16:52:30Z

extensions/amp-embedly-card/amp-embedly-card.md

+*Example: Embedding multiple resources*
+
+If you have a paid plan, use the `amp-embedly-key` component to set your api key. 
+You just need one per document that includes one or multiple `amp-embedly-card` components:


need "one" what? If it's amp-embedly-key, please specify that.

Like:

You just need one amp-embedly-key per AMP page.

Within the AMP page, you can specify one or multiple amp-embedly-card components:

ghost · 2018-06-12T16:55:52Z

extensions/amp-embedly-card/amp-embedly-card.md

+*Example: Embedding multiple resources*
+
+If you have a paid plan, use the `amp-embedly-key` component to set your api key. 
+You just need one per AMP page.


Clarify what you mean by "one". For example "You just need one amp-embedly-key per AMP page.

ghost · 2018-06-12T16:56:29Z

extensions/amp-embedly-card/amp-embedly-card.md

+</amp-embedly-key>
+```
+
+Setting a key through the `amp-embedly-key` component is optional. If you are a paid user, this removes Embedly's branding from the cards.


Would this be clearer:

If you are a paid user, setting the amp-embedly-key removes Embedly's branding from the cards.

Definitively, updated.

ghost · 2018-06-12T16:58:55Z

extensions/amp-embedly-card/amp-embedly-card.md

+
+##### data-card-theme
+
+For dark backgrounds it's better to specify the dark theme.


What are the possible values for this attribute. Should also state what this attribute does, which is "Sets the card theme." then tell them what the default is, and possible values.

Updated with possible values and default and more info on this attribute.

ghost · 2018-06-12T16:59:16Z

extensions/amp-embedly-card/amp-embedly-card.md

+
+##### data-card-image
+
+Specifies which image to use in article cards.


What should the value be - a URL, something else? Please advise.

Added more info.

ghost · 2018-06-12T16:59:41Z

extensions/amp-embedly-card/amp-embedly-card.md

+
+##### data-card-controls
+
+Enables share icons. The default value is `1`.


Clarify that by default share icons are enabled. To turn them off, specify '0'.?

Updated to explain these better.

ghost · 2018-06-12T16:59:53Z

extensions/amp-embedly-card/amp-embedly-card.md

+
+##### data-card-align
+
+Aligns the card. The default value is `"center"`.


What are the other values - please specify.

Added possible values.

ghost · 2018-06-12T17:00:21Z

extensions/amp-embedly-card/amp-embedly-card.md

+
+##### data-card-recommend
+
+Disables Embedly recommendations on video and rich cards. The default value is `1`.


Indicate that by default, recommendations are disabled (assuming "1" means that).

codecov-io · 2018-07-09T04:40:32Z

Codecov Report

Merging #14819 into master will decrease coverage by 0.03%.
The diff coverage is 56.36%.

@@            Coverage Diff             @@
##           master   #14819      +/-   ##
==========================================
- Coverage   77.61%   77.58%   -0.04%     
==========================================
  Files         553      557       +4     
  Lines       40337    40392      +55     
==========================================
+ Hits        31309    31339      +30     
- Misses       9028     9053      +25

Flag	Coverage Δ
#integration_tests	`32.68% <ø> (ø)`	⬆️
#unit_tests	`77.15% <56.36%> (-0.04%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 920e13f...3f18a27. Read the comment docs.

juanlizarazo · 2018-07-09T04:42:05Z

@bpaduch Updated with requested changes!

amp-embedly-card: uses requires extensio amp-embedly-card: Update amp-embedly-key validation rules

…ve satisfies)

…ort.

ghost

Docs LGTM

aghassemi · 2018-07-16T16:54:49Z

Yay! Thanks @juanlizarazo 🎉

aghassemi · 2018-07-30T17:00:43Z

@juanlizarazo This is in Dev Channel right now (If you enabled Dev Channel in https://cdn.ampproject.org/experiments.html you can give it a try). Will make it to production over the next couple of days.

juanlizarazo · 2018-07-31T14:51:03Z

Just tried it with different embeds. Can't wait!

gctommy · 2018-08-03T04:14:42Z

@aghassemi — the documentation should really mentioned that data-url (required) only accepts https protocol, otherwise your page won't validate.

juanlizarazo · 2018-08-03T04:44:15Z

@gctommy @aghassemi I can create a PR fixing that, I made sure the examples were compliant but I spaced out with the docs.

I will fix a typo as well I saw there too and possibly remove the data-via property as well. I talked to embedly support about it and the sdk doesn't do what it is supposed to. So right now using data-via does nothing.

googlebot added the cla: yes label Apr 24, 2018

juanlizarazo mentioned this pull request Apr 24, 2018

ey✨ implement amp-embedly component #13843

Closed

12 tasks

juanlizarazo force-pushed the 12908-amp-embedly-card branch from 51f871c to d298d76 Compare April 24, 2018 05:40

juanlizarazo mentioned this pull request Apr 24, 2018

Add support for amp-embedly #12908

Closed

cathyxz requested a review from alanorozco April 24, 2018 18:13

juanlizarazo force-pushed the 12908-amp-embedly-card branch from 25ed7e1 to e3cac88 Compare April 26, 2018 00:36

juanlizarazo force-pushed the 12908-amp-embedly-card branch from e3cac88 to bc2be81 Compare April 27, 2018 02:46

alanorozco requested changes May 2, 2018

View reviewed changes

aghassemi mentioned this pull request May 4, 2018

amp-reddit embeds don't resize correctly #11869

Closed

aghassemi assigned alanorozco May 30, 2018

juanlizarazo force-pushed the 12908-amp-embedly-card branch from bc2be81 to efa154d Compare June 11, 2018 04:14

alanorozco requested changes Jun 11, 2018

View reviewed changes

honeybadgerdontcare reviewed Jun 11, 2018

View reviewed changes

juanlizarazo commented Jun 12, 2018

View reviewed changes

alanorozco approved these changes Jun 12, 2018

View reviewed changes

ghost suggested changes Jun 12, 2018

View reviewed changes

juanlizarazo force-pushed the 12908-amp-embedly-card branch from 7260c9f to e613e6b Compare July 9, 2018 04:18

juanlizarazo and others added 17 commits July 14, 2018 19:54

amp-embedly-card: Update protoascii rules

f4f3544

amp-embedly-card: uses requires extensio amp-embedly-card: Update amp-embedly-key validation rules

amp-embedly-card: Move and rename validator html and out files

efee91d

amp-embedly-card: update validation rules (unique tag, protocol, remo…

aa69e53

…ve satisfies)

amp-embedly-card: Update validator test cases

f83fd97

amp-embedly-card: Clean up attributes in documentation

640a2b3

amp-embedly-card: Update jsdoc description

f28525c

touchups

32da3c8

touchup2

31c0ddf

amp-embedly-card: Initialize card correctly. Add data-card-embed supp…

7ce1743

…ort.

amp-embedly-card: Update documentation.

f943e9c

amp-embedly-card: Support embedly dark theme.

549c1a5

amp-embedly-card: Update theme documentation.

3fc9795

amp-embedly-card: Update data-card-theme docs

478d900

amp-embedly-card: Update data-card-theme docs

3f97841

amp-embedly-card: Add entry to bundles.config

3fc98a0

amp-embedly-card: update comment

d240129

amp-embedly-card: update docs

265be3c

juanlizarazo force-pushed the 12908-amp-embedly-card branch from ed97fc4 to 265be3c Compare July 15, 2018 02:07

juanlizarazo and others added 2 commits July 14, 2018 20:33

amp-embedly-card: Remove lint so tests pass

a47cd2e

touchups

3f18a27

ghost approved these changes Jul 16, 2018

View reviewed changes

alanorozco approved these changes Jul 16, 2018

View reviewed changes

alanorozco merged commit ae138bc into ampproject:master Jul 16, 2018

juanlizarazo deleted the 12908-amp-embedly-card branch July 17, 2018 15:09

Gregable pushed a commit that referenced this pull request Jul 17, 2018

cl/204782076 Revision bump for #14819

5238220


		##### data-card-theme

		For dark backgrounds it's better to specify the dark theme.


		##### data-card-image

		Specifies which image to use in article cards.


		##### data-card-controls

		Enables share icons. The default value is `1`.


		##### data-card-align

		Aligns the card. The default value is `"center"`.


		##### data-card-recommend

		Disables Embedly recommendations on video and rich cards. The default value is `1`.

		@@ -0,0 +1 @@
		PASS

✨ implement amp-embedly-card component #14819

✨ implement amp-embedly-card component #14819

Conversation

juanlizarazo commented Apr 24, 2018

juanlizarazo commented Apr 24, 2018

aghassemi commented Apr 26, 2018

juanlizarazo commented Apr 26, 2018

aghassemi commented Apr 27, 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

aghassemi commented May 30, 2018

juanlizarazo commented Jun 11, 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

juanlizarazo left a comment

Choose a reason for hiding this comment

alanorozco left a comment

Choose a reason for hiding this comment

alanorozco commented Jun 12, 2018

juanlizarazo commented Jun 12, 2018

alanorozco commented Jun 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 9, 2018 • edited Loading

Codecov Report

juanlizarazo commented Jul 9, 2018

ghost left a comment

Choose a reason for hiding this comment

aghassemi commented Jul 16, 2018

aghassemi commented Jul 30, 2018

juanlizarazo commented Jul 31, 2018 • edited Loading

gctommy commented Aug 3, 2018

juanlizarazo commented Aug 3, 2018

codecov-io commented Jul 9, 2018 •

edited

Loading

juanlizarazo commented Jul 31, 2018 •

edited

Loading