Added page URL fragment parameter #20238

bramanudom · 2019-01-09T23:45:09Z

#20083

A fragment parameter should be added to a story url, which should allow users to open (and eventually share) a story at a specific page.

gmajoulet · 2019-01-10T15:54:19Z

extensions/amp-story/1.0/amp-story.js

+            maybeInitialPageId = this.maybeChangeInitial_(initialPageId);
+          }
+          this.switchTo_(maybeInitialPageId, NavigationDirection.NEXT);
+        })


Chaining promises is pretty confusing, but here you're actually changing the way this code behaves.

The this.switchTo_() method returns a promise, that resolves when the page navigation happened.

The way the code was written was using an arrow function, with an implicit return
() => 'foo' is the shorthand for () => {return 'foo';}
What it means is that the way it was written before, it was returning the Promise from the switchTo_ method, and the next then() callback wasn't executed until the Promise was resolved (navigation happens).
Your new code doesn't return that Promise, which means that the following code will run before it's actually resolved.

Thank you for catching this and for taking the time to explain everything out! I made changes, but will definitely keep in mind the implicit return and be more perceptive about chained promises in the future.

gmajoulet · 2019-01-10T15:57:47Z

extensions/amp-story/1.0/amp-story.js

+   * @return {string}
+   * @private
+   */
+  maybeChangeInitial_(initialPageId) {


With these changes, the way we get the initial page ID is now done in two steps:

const initialPageId = this.getHistoryState_(HistoryStates.PAGE_ID) || firstPageEl.id;

Later, maybe changing it

What do you think of trying to do this in only one step? It might make things easier.
Like, where we get the initialPageId right now, we could have a getInitialPageId_() method, that would try to get the page ID in that order:

If there is a page ID in the history, use it (if a user refreshes their page, they'd expect to end up on the same page)

If there's a page ID in the URL fragment, use it

Fallback to the first page of the story

What do you think?

This makes sense to me! I went ahead and made the changes. maybeChangeInitial_ is now getInitialPageId and retrieves the correct pageId in the order you described. There's some code left because this is still behind an experiment wall but I mark that as a TODO to be removed explicitly.

Enriqe · 2019-01-10T16:01:50Z

extensions/amp-story/1.0/test/test-amp-story.js

@@ -1328,6 +1328,28 @@ describes.realWin('amp-story', {
                expect(story.activePage_.element.id).to.equal('cover');
              });
        });
+    it('should begin at the specified page fragment parameter value', () => {


nit: newline before this it() and the one below.

cvializ

LGTM, deferring to @gmajoulet and @Enriqe

gmajoulet · 2019-01-11T17:04:20Z

extensions/amp-story/1.0/amp-story.js

@@ -764,6 +765,8 @@ export class AmpStory extends AMP.BaseElement {
        this.element.querySelector('amp-story-page'),
        'Story must have at least one page.');

+    // TODO(#20128): initialPageId will no longer be necessary here once
+    // branching experiment is live.
    const initialPageId = this.getHistoryState_(HistoryStates.PAGE_ID) ||


Can we call const initialPageId = this.getInitialPageId_(firstPageEl); directly here, and do the experiment check in that method? This way, we could remove that block before the switchTo_, and keep the code like it was before.

I believe that getInitialPageId_ needs to be called later because at this point the pages_ array hasn't been initialized yet.

How do feel about getting rid of the assignment of initialPageId entirely, still moving the experiment check to getInitialPageID_, and calling it in switchTo_ directly as the first argument?

gmajoulet · 2019-01-11T17:05:21Z

extensions/amp-story/1.0/amp-story.js

+   * Retrieves the initial pageId to begin the story with. In order, the
+   * initial page for a story should be the page ID in the history, a valid
+   * page ID in the URL fragment, and then the first page of the story.
+   * @param {Element} firstPageEl


Nit: !Element

gmajoulet · 2019-01-11T17:15:59Z

extensions/amp-story/1.0/amp-story.js

+  /**
+   * Retrieves the initial pageId to begin the story with. In order, the
+   * initial page for a story should be the page ID in the history, a valid
+   * page ID in the URL fragment, and then the first page of the story.


With your implementation, the page ID in the URL hash will actually override anything else. But I think it's fine, and more aligned with other Google products. For example, on most Google products, a hl=language parameter in a URL would override any local preference.

+1 to param taking precedence over history state

gmajoulet · 2019-01-11T17:19:39Z

extensions/amp-story/1.0/amp-story.js

+    const maybePageId = parseQueryString(this.win.location.hash)['page'];
+    const isActualPage =
+      findIndex(this.pages_, page => page.element.id === maybePageId) >= 0;
+    return (maybePageId && isActualPage) ? maybePageId : initialPageId;


Could we also check that the page ID from the history state exists? This is a bug we're tracking too.

Can we use getPageIndexById instead, to avoid code duplication?
I was thinking wrapping these calls in try/catch statements might allow us to use this method (does it?), and still notify the developer that there is something wrong with what they're doing.

I think I'm a little confused here. By checking that the page ID from the history state exists, did you mean something more explicit than just this.getHistoryState_(HistoryStates.PAGE_ID) || firstPageEl.id?

Also, I don't think we can use a try/catch here because of how user().error() works. Just to double check I went ahead and tried, and code in the catch block does not execute despite getPageIndexById returning a negative number.

Actually, that line of code checks if there is something in the history (this.getHistoryState_(HistoryStates.PAGE_ID)), and if not (if that first expression is falsy), it fallbacks the ID of the first page of the story (firstPageEl.id).

We're getting a page ID from the history, but just like the page ID from the URL, we can't be sure that the story actually has a page for that ID. Because you just wrote some code to check this, I was wondering if we could also use it to check that the page ID from the history points to an existing page?

gmajoulet · 2019-01-15T23:51:07Z

extensions/amp-story/1.0/amp-story.js

+      if (maybePageId && isActualPage(maybePageId)) {
+        return maybePageId;
+      }
+    } else if (historyPage && isActualPage(historyPage)) {


We want to get the page from the history even if the branching experiment is on :))

Oops, thank you for catching this!

gmajoulet · 2019-01-16T16:15:01Z

extensions/amp-story/1.0/amp-story.js

+      if (maybePageId && isActualPage(maybePageId)) {
+        return maybePageId;
+      }
+    } if (historyPage && isActualPage(historyPage)) {


Plz fix the indent, and let's merge this :)) Thanks for implementing the comments!

bramanudom · 2019-01-18T18:26:11Z

@gmajoulet I believe this is ready for either continued review or for merging! 😀

* parse URl fragment parameter for starting page of the story * cleaned up code and fixed logic * fixed tests! made it harder for myself unnecessarily * removed logging and linted * removed logging and linted * changed to array util findIndex * addressed PR comments * edit JSDoc to reflect changes, added explicit todo * addressed PR comments * removed private getHistoryState_ * historystate not historystates :'( * missing arg * allow usage of history PageID when not branching * allow usage of history PageID when not branching

bramanudom added 9 commits January 8, 2019 16:13

parse URl fragment parameter for starting page of the story

4f418f3

cleaned up code and fixed logic

c8be39a

Merge branch 'master' into URL

466f7ff

fixed tests! made it harder for myself unnecessarily

769394a

Merge branch 'master' into URL

e071beb

removed logging and linted

591a8fb

removed logging and linted

91a37e7

fixed merge conflicts

f2febd8

fixed merge conflicts

603e78f

bramanudom requested review from gmajoulet, newmuis, cvializ and Enriqe January 9, 2019 23:45

googlebot added the cla: yes label Jan 9, 2019

changed to array util findIndex

02b8803

gmajoulet reviewed Jan 10, 2019

View reviewed changes

Enriqe reviewed Jan 10, 2019

View reviewed changes

cvializ reviewed Jan 10, 2019

View reviewed changes

bramanudom added 2 commits January 10, 2019 17:06

addressed PR comments

26849ed

edit JSDoc to reflect changes, added explicit todo

9e45b5c

gmajoulet reviewed Jan 11, 2019

View reviewed changes

bramanudom added 5 commits January 15, 2019 15:12

addressed PR comments

0061104

fixed merge conflicts

6fe41e0

removed private getHistoryState_

e1a268d

historystate not historystates :'(

feb9921

missing arg

91c4c47

gmajoulet approved these changes Jan 15, 2019

View reviewed changes

allow usage of history PageID when not branching

1387e87

gmajoulet reviewed Jan 16, 2019

View reviewed changes

allow usage of history PageID when not branching

021dd15

fixed tests and indenting

5d8ce6f

gmajoulet merged commit 304b48b into ampproject:master Jan 18, 2019

ampprojectbot added the PR use: In Beta / Experimental label Jan 24, 2019

bramanudom mentioned this pull request Feb 11, 2019

✨ Add page sharing options for branching #20777

Merged

newmuis mentioned this pull request Feb 21, 2019

AMP-STORY : Page ID change freezes the story totally! #19431

Closed

newmuis mentioned this pull request Jul 18, 2019

Allow accessing individual story pages by URL #12182

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added page URL fragment parameter #20238

Added page URL fragment parameter #20238

bramanudom commented Jan 9, 2019

gmajoulet Jan 10, 2019 •

edited

bramanudom Jan 10, 2019

gmajoulet Jan 10, 2019

bramanudom Jan 10, 2019

Enriqe Jan 10, 2019

bramanudom Jan 10, 2019

cvializ left a comment

gmajoulet Jan 11, 2019

bramanudom Jan 14, 2019 •

edited

gmajoulet Jan 11, 2019

gmajoulet Jan 11, 2019

newmuis Jan 12, 2019

gmajoulet Jan 11, 2019

bramanudom Jan 14, 2019

gmajoulet Jan 14, 2019

gmajoulet Jan 15, 2019

bramanudom Jan 16, 2019

gmajoulet Jan 16, 2019

bramanudom commented Jan 18, 2019

Added page URL fragment parameter #20238

Added page URL fragment parameter #20238

Conversation

bramanudom commented Jan 9, 2019

gmajoulet Jan 10, 2019 • edited

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

cvializ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bramanudom Jan 14, 2019 • edited

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

bramanudom commented Jan 18, 2019

gmajoulet Jan 10, 2019 •

edited

bramanudom Jan 14, 2019 •

edited