Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Story branching navigation path fixes. #24979

Merged
merged 2 commits into from
Oct 9, 2019
Merged

Story branching navigation path fixes. #24979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9d6c4b
Story navigation path fixes.
gmajoulet Oct 8, 2019
8030408
Reviews.
gmajoulet Oct 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions extensions/amp-story/1.0/amp-story-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,20 @@ export class AmpStoryPage extends AMP.BaseElement {
return this.element.getAttribute('i-amphtml-return-to');
}

const navigationPath = this.storeService_.get(
StateProperty.NAVIGATION_PATH
);
// Navigation path is a complete list of the navigation history, including
// the currently active page. The previous page is the next to last element
// of that array.
const previousPageId = navigationPath[navigationPath.length - 2];
gmajoulet marked this conversation as resolved.
Show resolved Hide resolved

if (previousPageId) {
return previousPageId;
}

// If the page was loaded with a `#page=foo` hash, it could have no
// navigation path but still a previous page in the DOM.
const previousElement = this.element.previousElementSibling;
if (previousElement && previousElement.tagName.toLowerCase() === TAG) {
return previousElement.id;
Expand Down
16 changes: 12 additions & 4 deletions extensions/amp-story/1.0/amp-story-store-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,9 @@ export const StateProperty = {
CURRENT_PAGE_ID: 'currentPageId',
CURRENT_PAGE_INDEX: 'currentPageIndex',
ADVANCEMENT_MODE: 'advancementMode',
PAGE_IDS: 'pageIds',
NAVIGATION_PATH: 'navigationPath',
NEW_PAGE_AVAILABLE_ID: 'newPageAvailableId',
PAGE_IDS: 'pageIds',
};

/** @private @const @enum {string} */
Expand All @@ -161,7 +162,8 @@ export const Action = {
CHANGE_PAGE: 'setCurrentPageId',
SET_CONSENT_ID: 'setConsentId',
SET_ADVANCEMENT_MODE: 'setAdvancementMode',
SET_PAGE_IDS: 'addToPageIds',
SET_NAVIGATION_PATH: 'setNavigationPath',
SET_PAGE_IDS: 'setPageIds',
TOGGLE_ACCESS: 'toggleAccess',
TOGGLE_AD: 'toggleAd',
TOGGLE_AFFILIATE_LINK: 'toggleAffiliateLink',
Expand Down Expand Up @@ -191,14 +193,15 @@ export const Action = {
* @private @const {!Object<string, !function(*, *):boolean>}
*/
const stateComparisonFunctions = {
[StateProperty.PAGE_IDS]: (old, curr) => old.length !== curr.length,
[StateProperty.ACTIONS_WHITELIST]: (old, curr) => old.length !== curr.length,
[StateProperty.INTERACTIVE_COMPONENT_STATE]:
/**
* @param {InteractiveComponentDef} old
* @param {InteractiveComponentDef} curr
*/
(old, curr) => old.element !== curr.element || old.state !== curr.state,
[StateProperty.NAVIGATION_PATH]: (old, curr) => old.length !== curr.length,
[StateProperty.PAGE_IDS]: (old, curr) => old.length !== curr.length,
};

/**
Expand Down Expand Up @@ -355,6 +358,10 @@ const actions = (state, action, data) => {
return /** @type {!State} */ (Object.assign({}, state, {
[StateProperty.ADVANCEMENT_MODE]: data,
}));
case Action.SET_NAVIGATION_PATH:
return /** @type {!State} */ (Object.assign({}, state, {
[StateProperty.NAVIGATION_PATH]: data,
}));
case Action.SET_PAGE_IDS:
return /** @type {!State} */ (Object.assign({}, state, {
[StateProperty.PAGE_IDS]: data,
Expand Down Expand Up @@ -489,8 +496,9 @@ export class AmpStoryStoreService {
[StateProperty.CURRENT_PAGE_ID]: '',
[StateProperty.CURRENT_PAGE_INDEX]: 0,
[StateProperty.ADVANCEMENT_MODE]: '',
[StateProperty.PAGE_IDS]: [],
[StateProperty.NEW_PAGE_AVAILABLE_ID]: '',
[StateProperty.NAVIGATION_PATH]: [],
[StateProperty.PAGE_IDS]: [],
});
}

Expand Down
83 changes: 35 additions & 48 deletions extensions/amp-story/1.0/amp-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,6 @@ export class AmpStory extends AMP.BaseElement {
/** @private @const {!Array<!./amp-story-page.AmpStoryPage>} */
this.adPages_ = [];

/** @private {Array<string>} */
this.storyNavigationPath_ = [];

/** @const @private {!./variable-service.AmpStoryVariableService} */
this.variableService_ = getVariableService(this.win);

Expand Down Expand Up @@ -951,9 +948,7 @@ export class AmpStory extends AMP.BaseElement {
page.setState(PageState.NOT_ACTIVE);
this.upgradeCtaAnchorTagsForTracking_(page, index);
});
if (isExperimentOn(this.win, 'amp-story-branching')) {
this.initializeStoryNavigationPath_();
}
this.initializeStoryNavigationPath_();
})
.then(() => this.initializeBookend_())
.then(() => {
Expand Down Expand Up @@ -1346,10 +1341,7 @@ export class AmpStory extends AMP.BaseElement {
* @private
*/
switchTo_(targetPageId, direction) {
const targetPage = isExperimentOn(this.win, 'amp-story-branching')
? this.updateStoryNavigationPath_(targetPageId, direction)
: this.getPageById(targetPageId);

const targetPage = this.getPageById(targetPageId);
const pageIndex = this.getPageIndex(targetPage);

// Step out if trying to navigate to the currently active page.
Expand Down Expand Up @@ -1378,6 +1370,9 @@ export class AmpStory extends AMP.BaseElement {

const oldPage = this.activePage_;
this.activePage_ = targetPage;
if (!targetPage.isAd()) {
this.updateNavigationPath_(targetPageId, direction);
}

// Each step will run in a requestAnimationFrame, and wait for the next
// frame before executing the following step.
Expand Down Expand Up @@ -1502,40 +1497,32 @@ export class AmpStory extends AMP.BaseElement {
}

/**
* Update the story level stack and check for navigation adherence
* to the path a user takes.
* Updates the story navigation stack and checks for navigation adherence to
gmajoulet marked this conversation as resolved.
Show resolved Hide resolved
* the path a user takes.
* @param {string} targetPageId
* @param {string} direction
* @return {!./amp-story-page.AmpStoryPage}
* @param {!NavigationDirection} direction
* @private
*/
updateStoryNavigationPath_(targetPageId, direction) {
updateNavigationPath_(targetPageId, direction) {
const navigationPath = /** @type {!Array<string>} */ (this.storeService_.get(
StateProperty.NAVIGATION_PATH
));

if (direction === NavigationDirection.PREVIOUS) {
this.storyNavigationPath_.pop();
if (this.storyNavigationPath_.length > 0) {
const pathPrevious = this.storyNavigationPath_[
this.storyNavigationPath_.length - 1
];
if (pathPrevious !== targetPageId) {
return this.getPageById(pathPrevious);
}
}
} else if (direction === NavigationDirection.NEXT) {
const topOfStack = this.storyNavigationPath_[
this.storyNavigationPath_.length - 1
];
// 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_.push(targetPageId);
}
navigationPath.pop();
}
setHistoryState(
this.win,
HistoryState.NAVIGATION_PATH,
this.storyNavigationPath_
);
return this.getPageById(targetPageId);

// Ensures the pageId is not at the top of the stack already, which can
// happen on initial page load (e.g. reloading a page).
if (
direction === NavigationDirection.NEXT &&
navigationPath[navigationPath.length - 1] !== targetPageId
) {
navigationPath.push(targetPageId);
}

this.storeService_.dispatch(Action.SET_NAVIGATION_PATH, navigationPath);
setHistoryState(this.win, HistoryState.NAVIGATION_PATH, navigationPath);
}

/**
Expand Down Expand Up @@ -2136,10 +2123,13 @@ export class AmpStory extends AMP.BaseElement {
}
// There may be other 1 skip away pages due to branching.
if (isExperimentOn(this.win, 'amp-story-branching')) {
const indexInStack = this.storyNavigationPath_.indexOf(
const navigationPath = this.storeService_.get(
StateProperty.NAVIGATION_PATH
);
const indexInStack = navigationPath.indexOf(
this.activePage_.element.id
);
const maybePrev = this.storyNavigationPath_[indexInStack - 1];
const maybePrev = navigationPath[indexInStack - 1];
if (indexInStack > 0 && pageId === this.activePage_.element.id) {
if (!pagesByDistance[1]) {
pagesByDistance[1] = [];
Expand Down Expand Up @@ -2545,21 +2535,18 @@ export class AmpStory extends AMP.BaseElement {
* @private
*/
initializeStoryNavigationPath_() {
const historyNavigationPath = getHistoryState(
this.win,
HistoryState.NAVIGATION_PATH
this.storeService_.dispatch(
Action.SET_NAVIGATION_PATH,
getHistoryState(this.win, HistoryState.NAVIGATION_PATH) || []
);
if (historyNavigationPath) {
this.storyNavigationPath_ = /** @type {!Array<string>} */ (historyNavigationPath);
}
}

/** @private */
replay_() {
if (this.storeService_.get(StateProperty.BOOKEND_STATE)) {
this.hideBookend_();
}
this.storyNavigationPath_.length = 0;
this.storeService_.dispatch(Action.SET_NAVIGATION_PATH, []);
const switchPromise = this.switchTo_(
dev().assertElement(this.pages_[0].element).id,
NavigationDirection.NEXT
Expand Down