✨ Navigation update with additions to the history state #20313
Conversation
| }; | ||
|
|
||
| /** @enum {string} */ | ||
| const HistoryStates = { |
There was a problem hiding this comment.
Please use HistoryState from utils.js. @gmajoulet made a change recently where we moved the history states to that file.
| page.setState(PageState.NOT_ACTIVE); | ||
| this.upgradeCtaAnchorTagsForTracking_(page, index); | ||
| }); | ||
| this.initializeStoryNavigationPath_(); |
There was a problem hiding this comment.
this feature is still under an experiment no?
If so, I would wrap this with the isExperimentOn(this.win, 'amp-story-branching'), in the case we rollback/promote the experiment it's easier to spot the code that needs to be changed/removed.
There was a problem hiding this comment.
Youre right. Thanks for catching this!
| setHistoryState( | ||
| this.win, | ||
| HistoryState.NAVIGATION_PATH, | ||
| this.storyNavigationPath_.map(page => page.element.id)); |
There was a problem hiding this comment.
Sorry to be picky on this, but we try to be extra careful with all the code that runs during navigation, since it delays the time where the next page will be visible.
Should we fill the this.storyNavigationPath_ array with page IDs in the first place, and do the getPageByById() in the switchTo method, using the ID this method returns?
This way we could skip that loop and just store this.storyNavigationPath_
| }); | ||
|
|
||
| it( | ||
| 'should navigate to the correct previous page after navigating away', |
| this.storyNavigationPath_.push(targetPage); | ||
| const topOfStack = | ||
| this.storyNavigationPath_[this.storyNavigationPath_.length - 1] || | ||
| this.storyNavigationPath_[0]; |
There was a problem hiding this comment.
Just curious: when could this.storyNavigationPath_[this.storyNavigationPath_.length - 1] be falsy? It would if the array is empty, but then this.storyNavigationPath_[0] would be falsy too?
There was a problem hiding this comment.
Thanks for catching this. Looking back now, I'm not sure what I was trying to do. Removed!
| this.storyNavigationPath_[this.storyNavigationPath_.length - 1] || | ||
| this.storyNavigationPath_[0]; | ||
| // // If the user navigates the away from the page, the top of storyStack | ||
| // // will be the same as ampStoryPageId in the history state. |
There was a problem hiding this comment.
Nit: comment in comment (xzibit.jpg)
| this.storyNavigationPath_[0]; | ||
| if (pathPrevious.element.id != targetPageId) { | ||
| return pathPrevious; | ||
| if (pathPrevious != targetPageId) { |
There was a problem hiding this comment.
Nit: here and below, please use strict equality checks
There was a problem hiding this comment.
I think you missed the strict equality check here :D
There was a problem hiding this comment.
Oof, thanks Enrique!
| // If the user navigates the away from the page, the top of storyStack | ||
| // will be the same as ampStoryPageId in the history state. | ||
| if ((targetPageId !== topOfStack) || | ||
| this.storyNavigationPath_.length === 0) { |
There was a problem hiding this comment.
It looks like if this.storyNavigationPath_.length is 0, topOfStack will be undefined.
So targetPageId !== topOfStack will always be true for this.storyNavigationPath_.length of 0.
Can we replace this if with just the first expression if (targetPageId !== topOfStack)? Lmk if I'm missing something!
|
@gmajoulet I believe this is ready for either continued review or for merging! 😀 |
…0313) * added updates to history for edge cases * added tests and JSDoc * added space before last it spec * removed extra arg * actually fix merge conflicts * added experiment check * formatting * addressed PR comments and fixed accidental double pushing bug * fix JSDoc param type * responded to PR comments * got rid of extra logical check, fixes tests * missed one strict equality check * fixed merge comnflicts in URL fragment paramter tests * lint, indentation
#20083
An update to #20182, this PR addresses the edge case where the user clicks through a story with branching, and then navigates away from the story. If the user were to navigate back to the story, for good UX, the story path taken prior to navigating away should be preserved.