Made changes to navigation to support branching #20182

bramanudom · 2019-01-07T23:51:43Z

#20083

Ideally when a user navigates back within a story, they go back to the page they came from.

For example, if a user is on page 4, then navigates to page 5, then goes back, they should go back to page 4. With branching, this still needs to be the case. That is, if a user is on page 1, then jumps to page 5, then goes back, they should go back to page 1.

This PR adds a story level stack that keeps track of the user's path while navigating through the story.

bramanudom · 2019-01-08T16:15:05Z

Note that the current unit test "should navigate back to the correct previous page after advance-to" is expected to fail until #20116 successfully merges.

Enriqe · 2019-01-08T16:47:21Z

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

+  updateStoryPath_(targetPageId, direction) {
+    const targetPage = this.getPageById(targetPageId);
+    if (direction == 'previous') {
+      this.storyPath_.pop();


curious: have you tried navigating to a page in the story, navigating outside of the story, clicking back in the browser history to re-enter the story, and then navigating to the previous page? Will that still work?

+1, we might want to store the storyPath_ in the browser history in a following PR.

Thanks for bringing this case to attention, @Enriqe . You're right, the expected previous navigation is broken when the user navigates out of and comes back to the story.

I will send a follow-up PR that updates the browser history like @gmajoulet suggested here and offline.

cvializ · 2019-01-08T18:46:24Z

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

  /**
   * Switches to a particular page.
   * @param {string} targetPageId
+   * @param {string} direction


If the direction will only ever be a limited set of options, consider using a Closure Compiler @enum type instead of a raw string.

amphtml/extensions/amp-story/1.0/page-advancement.js

Lines 42 to 46 in 5e57e9f

/** @const @enum */

export const TapNavigationDirection = {

'NEXT': 1,

'PREVIOUS': 2,

};

Done, in the amp-story-page level.

gmajoulet · 2019-01-08T16:59:52Z

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

        const {args} = invocation;
        if (args) {
-          this.switchTo_(args['id']);
+          this.switchTo_(args['id'], 'next');


Can we have an enum to store these previous and next values? This way, we'd get compilation errors instead of silent errors if we have a typo :)

gmajoulet · 2019-01-08T17:00:59Z

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

    this.adPages_ = [];

+    /** @private @const {!Array<!./amp-story-page.AmpStoryPage>} */
+    this.storyPath_ = [];


Nit: maybe storyNavigationPath? In this ever growing file, it's useful to give as much context as possible...

gmajoulet · 2019-01-08T17:02:05Z

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

   * @param {string} targetPageId
+   * @param {string} direction
   * @return {!Promise}
   */


Nit: not your doing, but could you please add a @private here? Thanks!

gmajoulet · 2019-01-08T17:02:28Z

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

    const oldPage = this.activePage_;
    this.activePage_ = targetPage;

+


Nit: no extra new line

gmajoulet · 2019-01-08T17:02:55Z

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

+   */
+  updateStoryPath_(targetPageId, direction) {
+    const targetPage = this.getPageById(targetPageId);
+    if (direction == 'previous') {


Nit: please use strict equality checks, here and below

gmajoulet · 2019-01-08T18:51:19Z

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

+   * to the path a user takes.
+   * @param {string} targetPageId
+   * @param {string} direction
+   * @private


Nit: @return {!AmpStoryPage}

gmajoulet · 2019-01-08T18:56:45Z

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

+  updateStoryPath_(targetPageId, direction) {
+    const targetPage = this.getPageById(targetPageId);
+    if (direction == 'previous') {
+      this.storyPath_.pop();


+1, we might want to store the storyPath_ in the browser history in a following PR.

gmajoulet · 2019-01-08T18:59:30Z

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

+
+    it('should navigate back to the correct previous page after goToPage',
+        () => {
+          toggleExperiment(win, 'amp-story-branching', true);


You can enable this experiment for all your describe block, by adding before(() => {/* enable experiment */}) and after(() => {/* Disable experiment */})

gmajoulet · 2019-01-08T19:01:31Z

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

+                    new MouseEvent('click', {clientX: 0}));
+                expect(story.activePage_.element.id).to.equal('cover');
+
+                toggleExperiment(win, 'amp-story-branching', false);


Ideally, a test would only assert one thing, like moving forwards, or moving backwards. Could you please split this test in two tests? It's going to create duplicated code, but in the context of tests it'd actually help debugging, as you'd know right away if you broke forward or backward navigation.

This makes sense. I went ahead and removed the expects confirms the correct forwards movement. The advance-to PR itself should already have a test that tests this.

gmajoulet · 2019-01-08T19:01:42Z

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

+                    .element.setAttribute('advance-to', 'page-3');
+
+                expect(story.activePage_.element.id).to.equal('cover');
+                // Move forwards


Nit: formatting // Moves forwards.

gmajoulet

Great!!

gmajoulet · 2019-01-08T21:49:20Z

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


  /**
   * @param {string} targetPageId
+   * @param {NavigationDirection} direction


Nit: {!NavigationDirection}

gmajoulet · 2019-01-08T21:50:04Z

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

  /**
   * Switches to a particular page.
   * @param {string} targetPageId
+   * @param {string} direction


Nit: {!NavigationDirection}

gmajoulet · 2019-01-08T21:52:21Z

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

  describe('amp-story branching', () => {
+
+    beforeEach(() => {toggleExperiment(win, 'amp-story-branching', true);});
+    afterEach(() => {toggleExperiment(win, 'amp-story-branching', false);});


Here you could use either before/afterEach() or before/after(). The first one is going to be executed before/after each test, when the first one is going to be executed only once before all the tests, and then once after all the tests. I think we might want the latter in this very use case :)

Synced offline, but the tests always fail when using before/after, reproduce-ably. Seems to be a bug!

bramanudom · 2019-01-09T20:38:47Z

I think this is ready for merging! 🌴 (for branching) @gmajoulet

* added story level stack * wip: added logic for pushing/ popping * cleaned up code * added tests * added experiment check * remove console.log * fixed type error * addressed PR comments * addressed PR comments

bramanudom added 7 commits January 2, 2019 11:13

added story level stack

ad92e7b

wip: added logic for pushing/ popping

6d796f1

cleaned up code

d4335b2

Merge branch 'master' into navigation

fba19f3

added tests

2ef5841

added experiment check

bd7a14e

remove console.log

edba9ba

bramanudom requested review from gmajoulet, newmuis, cvializ and Enriqe January 7, 2019 23:51

googlebot added the cla: yes label Jan 7, 2019

fixed type error

7e6a0a4

Enriqe reviewed Jan 8, 2019

View reviewed changes

cvializ reviewed Jan 8, 2019

View reviewed changes

gmajoulet reviewed Jan 8, 2019

View reviewed changes

addressed PR comments

ef0b46e

gmajoulet approved these changes Jan 8, 2019

View reviewed changes

bramanudom added 2 commits January 9, 2019 12:01

addressed PR comments

0f14ea6

fixed merge conflicts

55534ee

gmajoulet merged commit 7c16757 into ampproject:master Jan 9, 2019

bramanudom mentioned this pull request Jan 14, 2019

✨ Navigation update with additions to the history state #20313

Merged

bramanudom mentioned this pull request Jan 31, 2019

✨Amend preloading to work with branching #20611

Merged

	/** @const @enum */
	export const TapNavigationDirection = {
	'NEXT': 1,
	'PREVIOUS': 2,
	};

		const oldPage = this.activePage_;
		this.activePage_ = targetPage;

Made changes to navigation to support branching #20182

Made changes to navigation to support branching #20182

Uh oh!

Conversation

bramanudom commented Jan 7, 2019

Uh oh!

bramanudom commented Jan 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bramanudom commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bramanudom commented Jan 9, 2019 •

edited

Loading