Implement bookend component for amp-story #11526

alanorozco · 2017-10-02T17:33:56Z

From newmuis/amphtml-story#49, newmuis/amphtml-story#54, newmuis/amphtml-story#56, newmuis/amphtml-story#69, newmuis/amphtml-story#183

/cc @newmuis

dvoytenko · 2017-10-02T21:24:11Z

extensions/amp-story/0.1/bookend-share.js

+
+
+const LINK_SHARE_TEMPLATE =
+    `<div class="i-amphtml-story-share-icon">


We don't allow XSS-able inner HTMLs. Please just use the DOM API.

dvoytenko · 2017-10-02T21:24:30Z

extensions/amp-story/0.1/bookend-share.js

+    `<div class="i-amphtml-story-share-icon">
+      ${ICONS.link}
+    </div>
+    <span class="i-amphtml-story-share-name">Get link</span>`;


Ok to go w/o internationalization for now?

Yeah. Added a TODO

dvoytenko · 2017-10-02T21:26:09Z

extensions/amp-story/0.1/bookend-share.js

+  const icon = type == 'email' ? ICONS.mail : '';
+
+  return (
+      `<amp-social-share


Ditto. DOM API.

dvoytenko · 2017-10-02T21:26:21Z

extensions/amp-story/0.1/bookend-share.js

+  const params = !opt_params ? '' :
+      Object.keys(opt_params)
+          .map(field =>
+              `data-param-${escapeHtml(field)}=` +


DOM API. Then you won't need escapeHtml either.

dvoytenko · 2017-10-02T21:27:23Z

extensions/amp-story/0.1/bookend-share.js

+   */
+  buildItem_(html) {
+    const el = this.win_.document.createElement('li');
+    el./*OK*/innerHTML = html;


dvoytenko · 2017-10-02T21:27:55Z

extensions/amp-story/0.1/bookend.js

+ *   relatedArticles: !Array<!./related-articles.RelatedArticleSet>
+ * }}
+ */
+export let BookendConfig;


BookendConfigDef

dvoytenko · 2017-10-02T21:28:03Z

extensions/amp-story/0.1/bookend.js

+  // TODO(alanorozco): Consider using amp-img and what we need to get it working
+  const imgHtml = articleData.image ? (
+      `<div class="i-amphtml-story-bookend-article-image">
+        <img src="${articleData.image}"


dvoytenko · 2017-10-02T21:28:43Z

extensions/amp-story/0.1/related-articles.js

+/**
+ * @typedef {{title: string, url: string, image: (string|undefined)}}
+ */
+export let RelatedArticle;


Def here and below

dreamofabear · 2017-10-03T16:39:34Z

extensions/amp-story/0.1/bookend-share.js

+  });
+
+  // constant value, no XSS risk
+  iconEl./*OK*/innerHTML = ICONS.link;


Let's put a TODO and add this to #11545. Should be possible to create SVGs with pure JS.

Also, let's change icons.js to use non-string interpolation and add a type declaration.

change icons.js to use non-string interpolation and add a type declaration.

Done.

Should be possible to create SVGs with pure JS.

Should we? Seems like overkill. We could also just use compiled templates.

<template>? That would work too.

I made a notes above. But why isn't this in CSS?

dreamofabear · 2017-10-03T16:40:44Z

extensions/amp-story/0.1/bookend-share.js

+
+  if (type == 'email') {
+    // constant value, no XSS risk
+    shareEl./*OK*/innerHTML = ICONS.mail;


Ditto. Are these icons only used once each? If so, let's just inline them here.

I feel like that's a little messy :s

Ideally I think we should also move the system layer icons here.

dreamofabear · 2017-10-03T16:42:33Z

extensions/amp-story/0.1/bookend-share.js

+
+  /** @private */
+  loadRequiredExtensions_() {
+    Services.extensionsFor(this.win_).loadExtension('amp-social-share');


installExtensionForDoc. Are all STAMPs required to have a bookend? When will this be invoked?

Done.

All STAMPs are required to have a bookend, but not all bookends will have share buttons that require amp-social-share, since that is configurable.

It's also not guaranteed that the user will navigate to the bookend, so the extension is only loaded once Bookend sets share configuration for the share widget. This happens on "bookend preload time", which is executed once the user gets close enough to the widget.

This will be clear in the upcoming PR for the top-level AmpStory component.

dreamofabear · 2017-10-03T16:43:05Z

extensions/amp-story/0.1/bookend-share.js

+});
+
+
+function buildLinkShareItem(doc) {


Please document this function.

dreamofabear · 2017-10-03T16:43:14Z

extensions/amp-story/0.1/bookend-share.js

+}
+
+
+function buildProvider(doc, type, opt_params) {


Document please.

dreamofabear · 2017-10-03T16:43:22Z

extensions/amp-story/0.1/bookend-share.js

+}
+
+
+export class BookendShareWidget {


Please document this class.

dreamofabear · 2017-10-03T16:49:52Z

extensions/amp-story/0.1/bookend.js

+    const headingEl = createElementWithAttributes(this.win_.document, 'h3', {
+      'class': 'i-amphtml-story-bookend-heading',
+    });
+    headingEl./*OK*/innerText = escapeHtml(heading);


textContent. Also, why do we need to escapeHtml?

Done. We don't need to, fixed.

dreamofabear · 2017-10-03T16:51:27Z

extensions/amp-story/0.1/related-articles.js

+          articles: articleSetsResponse[headingKey].map(buildArticleFromJson_),
+        };
+
+        if (!!headingKey.trim().length) {


!! unnecessary.

dreamofabear · 2017-10-03T16:53:08Z

extensions/amp-story/0.1/related-articles.js

+function buildArticleFromJson_(articleJson) {
+  const article = {
+    title: user().assert(articleJson.title),
+    url: user().assert(articleJson.url),


PE follow-up: we should have useful error messaging and graceful failures. E.g. a missing title in one article shouldn't affect display of other articles.

Yup! Added a TODO, not super important right now.

dreamofabear · 2017-10-03T16:53:54Z

extensions/amp-story/0.1/related-articles.js

+ */
+// TODO(alanorozco): tighten externs
+// TODO(alanorozco): domain name
+export function buildFromJson(articleSetsResponse) {


relatedArticlesFromJson. Exports should have more context in their names.

dreamofabear

SEND IT

Obsolete

dvoytenko · 2017-10-03T20:45:15Z

extensions/amp-story/0.1/icons.js

-        <path d="M20 4H4c-1.1 0-1.99.9-1.99 2L2 18c0 1.1.9 2 2 2h16c1.1 0 2-.9 2-2V6c0-1.1-.9-2-2-2zm0 4l-8 5-8-5V6l8 5 8-5v2z"/>
-        <path d="M0 0h24v24H0z" fill="none"/>
-      </svg>`,
+      '<svg xmlns="http://www.w3.org/2000/svg" width="32px" height="32px" viewBox="0 0 24 24">' +


SVGs can definitely be created in JS. But a different question I have here: our approach so far been that we put all icon SVGs in CSS. Why not do it here as well?

Implement bookend component for amp-story

9894606

alanorozco requested review from dvoytenko and dreamofabear October 2, 2017 17:33

googlebot added the cla: yes label Oct 2, 2017

newmuis mentioned this pull request Oct 2, 2017

Send PRs to merge amphtml-story into the amphtml repo newmuis/amphtml-story#180

Closed

13 tasks

/*OK*/

f86d84b

dvoytenko previously requested changes Oct 2, 2017

View reviewed changes

ampprojectbot added this to the Pending Triage milestone Oct 2, 2017

Review

b94c07a

alanorozco force-pushed the story-bookend branch from 437966e to b94c07a Compare October 2, 2017 22:42

dreamofabear mentioned this pull request Oct 3, 2017

Implement system layer components for amp-story #11527

Merged

newmuis mentioned this pull request Oct 3, 2017

Add the core amp-story component #11511

Merged

dreamofabear reviewed Oct 3, 2017

View reviewed changes

Review

dc6b203

dreamofabear approved these changes Oct 3, 2017

View reviewed changes

alanorozco merged commit 2e3db6a into ampproject:master Oct 3, 2017

alanorozco deleted the story-bookend branch October 3, 2017 19:24

dvoytenko reviewed Oct 3, 2017

View reviewed changes



		const LINK_SHARE_TEMPLATE =
		`<div class="i-amphtml-story-share-icon">

Implement bookend component for amp-story #11526

Implement bookend component for amp-story #11526

Conversation

alanorozco commented Oct 2, 2017

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

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

dreamofabear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment