✨Add info dialog containing a link to the canonical URL to embedded stories #15130

newmuis · 2018-05-07T17:45:25Z

newmuis · 2018-05-07T17:45:39Z

Will wait for approval, then copy this to 1.0.

Enriqe · 2018-05-07T18:13:53Z

gulpfile.js

@@ -168,6 +169,7 @@ declareExtension('amp-story', '1.0', {
    'amp-story-hint',
    'amp-story-unsupported-browser-layer',
    'amp-story-viewport-warning-layer',
+    // TODO(newmuis): 'amp-story-info-dialog',


forgot to remove this todo?

oh nvm, I didn't see it was for 1.0.

Enriqe · 2018-05-07T18:22:52Z

extensions/amp-story/0.1/amp-story-info-dialog.js

+      return Promise.resolve();
+    }
+
+    return this.vsync_.runPromise({


Suggestion: We're trying to move away from vsync, and preferred method is baseElement.measureMutateElement() or using resources like seen here.

Enriqe · 2018-05-07T18:24:29Z

extensions/amp-story/0.1/amp-story-info-dialog.js

+/** @const {string} Class to toggle the info dialog link. */
+export const MOREINFO_VISIBLE_CLASS = 'i-amphtml-story-info-moreinfo-visible';
+
+export class InfoDialog {


Suggestion: add comment stating what this class is / does.

gmajoulet · 2018-05-07T19:13:39Z

extensions/amp-story/0.1/amp-story-info-dialog.js

+    createShadowRootWithStyle(root, this.element_, CSS);
+    this.initializeListeners_();
+
+    const appendPromise = this.vsync_.runPromise({


Apparently, we have to use resources instead of vsync now. It is going to log warnings/errors soon, we'll have to migrate everything anyways, so it's not a big deal if you don't want to switch here.

gmajoulet · 2018-05-07T19:15:47Z

extensions/amp-story/0.1/amp-story-info-dialog.js

+
+    const appendPromise = this.vsync_.runPromise({
+      mutate: () => {
+        this.innerContainerEl_ =


I learned from other team members the other day that since a DOM query doesn't trigger any relayout, we don't have to execute it in a measure callback

But more importantly, it looks like you mixed the mutate/measure keywords

gmajoulet · 2018-05-07T19:29:10Z

extensions/amp-story/0.1/amp-story-system-layer.js

+   */
+  onInfoClick_() {
+    const isOpen = this.storeService_.get(StateProperty.INFO_DIALOG_STATE);
+    this.storeService_.dispatch(Action.TOGGLE_INFO_DIALOG, !isOpen);


Super nitty / discussion:

Could we build the domain info dialog only when the user clicks this button to open it? I believe the code would be easy to write, but there may be edge cases I don't know about.
Would calling this.build().then(/* Add class to display the dialog */) from the store info dialog state listener work?

Opening the dialog becoming asynchronous, we then might want to get this onInfoClick method a way to open the dialog only, to make sure a user clicking twice on this button doesn't hide the dialog back.

I think it may be slightly more complicated due to the promise for the more info link. Mind if I add in a follow up PR?

Sounds good :)

gmajoulet · 2018-05-07T19:33:33Z

extensions/amp-story/0.1/test/test-amp-story-info-dialog.js

+  });
+
+  it('should build the info dialog', () => {
+    infoDialog.build().then(() => {


How does mocha know it has to wait to run the assertions here and below? Do we have to return the promise?

Whoops 😄

newmuis · 2018-05-11T00:33:10Z

Adding @choumx to review the usage of sendMessageAwaitResponse, since it requires whitelisting.

gmajoulet · 2018-05-11T14:41:23Z

extensions/amp-story/0.1/amp-story-system-layer.css

+  display: none !important;
+}
+
+.i-amphtml-embedded .i-amphtml-story-info-control {


I may be missing something here, but it looks like this wouldn't work because the i-amphtml-embedded class isn't set in the shadow DOM?

Apparently, all we'd have to do would be to add this class ourselves when building the system layer. Once we retrieve the viewer through viewerForDoc, there's this viewer.isEmbedded method.
Also, I think it'd be great to add this test in the amp-story file, so we don't even build the component if we don't need to

newmuis · 2018-05-11T17:58:02Z

PTAL

gmajoulet

Thanks for implementing all these suggestions :))

dreamofabear · 2018-05-11T21:11:18Z

extensions/amp-story/0.1/amp-story-info-dialog.js

+      this.setHeading_(),
+      this.setPageLink_(pageUrl),
+      this.requestMoreInfoLink_()
+          .then(moreInfoUrl => this.setMoreInfoLinkUrl_(moreInfoUrl)),


Don't know if you want to wait on this promise since it could take a long time if viewer times out.

The promise is actually only used for testing; not sure how best to denote that

Maybe add a comment on the @return line.

dreamofabear · 2018-05-11T21:17:42Z

extensions/amp-story/0.1/amp-story-info-dialog.js

+    return messagingPromise
+        .then(() => {
+          return this.viewer_.sendMessageAwaitResponse('moreInfoLinkUrl',
+              /* data */ undefined);


Do we need the runtime to ask for this URL or can it be provided by the viewer at load time via hash param? See viewer-impl.getParam().

This was considered, but decided against to keep the URL length down, since we don't strictly need this populated when the document is initialized (since the user is likely to not click the button that renders this UI until after the messaging has completed).

…variant

newmuis · 2018-05-30T00:21:58Z

I will migrate this to 1.0 in a separate PR.

newmuis requested review from gmajoulet and Enriqe May 7, 2018 17:45

googlebot added the cla: yes label May 7, 2018

Enriqe approved these changes May 7, 2018

View reviewed changes

gmajoulet reviewed May 7, 2018

View reviewed changes

newmuis requested a review from dreamofabear May 11, 2018 00:32

gmajoulet reviewed May 11, 2018

View reviewed changes

gmajoulet approved these changes May 11, 2018

View reviewed changes

dreamofabear reviewed May 11, 2018

View reviewed changes

newmuis force-pushed the amp-story-domain-dialog branch from 94ff4b2 to 2751140 Compare May 15, 2018 00:17

newmuis force-pushed the amp-story-domain-dialog branch from 2751140 to 56e180c Compare May 24, 2018 15:11

newmuis added 16 commits May 29, 2018 11:18

Base implementation for the info dialog in 0.1

d7431ee

Request more info link from viewer

594eaef

Remove debug logging

b7e1e66

Update styles

d197c59

Add unit tests

2f43b9d

Lint and fix types

fead7b6

Only show icon for embedded docs

56e1182

Revert JS version in helloworld.html

f770f25

Use resources instead of vsync.

1b90df0

Return promises from tests and fix tests

c0897ea

Add class-level comment

0bae8a1

Propagate the i-amphtml-embedded class into the system layer shadow DOM

5617dec

Only show icon on the story's first page.

f49c5fd

Move icon to leftmost slot in right button tray and change to stroke …

69b36ee

…variant

Fix embedded check

a7220d3

Do not build info dialog unless document is embedded.

20a57c8

newmuis added 3 commits May 29, 2018 11:18

Whitelist usage of sendMessageAwaitResponse

4eb9423

Add comment to clarify build promise is used for testing

d846a51

Instantiate info dialog at build time, since it is unused otherwise.

b890626

newmuis force-pushed the amp-story-domain-dialog branch from 56e180c to b890626 Compare May 29, 2018 18:56

newmuis added 4 commits May 29, 2018 16:01

Update store service tests

bc355fa

Register LocalizationService in share menu tests

7fd63c1

Register LocalizationService in amp-story tests

58754d6

Update amp-story test with new CHANGE_PAGE syntax

dcbfa01

newmuis merged commit bcc857c into ampproject:master May 30, 2018

newmuis mentioned this pull request Jun 6, 2018

♻️ Port amp-story info dialog to 1.0 #15872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Add info dialog containing a link to the canonical URL to embedded stories #15130

✨Add info dialog containing a link to the canonical URL to embedded stories #15130

newmuis commented May 7, 2018

newmuis commented May 7, 2018

Enriqe May 7, 2018

Enriqe May 7, 2018

Enriqe May 7, 2018

Enriqe May 7, 2018

gmajoulet May 7, 2018

gmajoulet May 7, 2018

gmajoulet May 7, 2018

newmuis May 11, 2018

gmajoulet May 11, 2018

gmajoulet May 7, 2018

newmuis May 11, 2018

newmuis commented May 11, 2018

gmajoulet May 11, 2018

newmuis May 11, 2018

newmuis commented May 11, 2018

gmajoulet left a comment

dreamofabear May 11, 2018

newmuis May 11, 2018

dreamofabear May 14, 2018

dreamofabear May 11, 2018

newmuis May 11, 2018

newmuis commented May 30, 2018

✨Add info dialog containing a link to the canonical URL to embedded stories #15130

✨Add info dialog containing a link to the canonical URL to embedded stories #15130

Conversation

newmuis commented May 7, 2018

newmuis commented May 7, 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

newmuis commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newmuis commented May 11, 2018

gmajoulet left a 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

newmuis commented May 30, 2018