From ecc62dd1c4c22e0bb309e2965a7ad94ed23dd9cc Mon Sep 17 00:00:00 2001 From: Gabriel Majoulet Date: Fri, 16 Mar 2018 12:08:12 -0400 Subject: [PATCH] Triggering the bookend through the store. (#14036) * Removing the bookend events to rely on the store and its actions. * Use synchronous store service. * More cleaning and unit tests. * Reviews. --- extensions/amp-story/0.1/amp-story-page.js | 3 +- .../amp-story/0.1/amp-story-store-service.js | 3 + extensions/amp-story/0.1/amp-story.js | 108 +++++++----------- extensions/amp-story/0.1/bookend.js | 31 +++-- extensions/amp-story/0.1/events.js | 6 - extensions/amp-story/0.1/navigation-state.js | 46 +++++--- .../amp-story/0.1/pagination-buttons.js | 56 +++++---- .../0.1/test/test-amp-story-store-service.js | 25 ++++ .../0.1/test/test-navigation-state.js | 14 ++- 9 files changed, 158 insertions(+), 134 deletions(-) diff --git a/extensions/amp-story/0.1/amp-story-page.js b/extensions/amp-story/0.1/amp-story-page.js index 67a84c859469..5c578ff163c2 100644 --- a/extensions/amp-story/0.1/amp-story-page.js +++ b/extensions/amp-story/0.1/amp-story-page.js @@ -594,8 +594,7 @@ export class AmpStoryPage extends AMP.BaseElement { next(opt_isAutomaticAdvance) { const pageId = this.getNextPageId(opt_isAutomaticAdvance); - if (pageId === null) { - dispatch(this.element, EventType.SHOW_BOOKEND, /* opt_bubbles */ true); + if (!pageId) { return; } diff --git a/extensions/amp-story/0.1/amp-story-store-service.js b/extensions/amp-story/0.1/amp-story-store-service.js index d8777dc4eb20..529f9bafe2cf 100644 --- a/extensions/amp-story/0.1/amp-story-store-service.js +++ b/extensions/amp-story/0.1/amp-story-store-service.js @@ -66,6 +66,9 @@ export const Action = { const actions = (state, action, data) => { switch (action) { case Action.TOGGLE_BOOKEND: + if (!state[StateProperty.CAN_SHOW_BOOKEND]) { + return state; + } return /** @type {!State} */ (Object.assign( {}, state, {[StateProperty.BOOKEND_STATE]: !!data})); default: diff --git a/extensions/amp-story/0.1/amp-story.js b/extensions/amp-story/0.1/amp-story.js index 37db676adef3..5f8f40a2d6f2 100644 --- a/extensions/amp-story/0.1/amp-story.js +++ b/extensions/amp-story/0.1/amp-story.js @@ -27,11 +27,15 @@ import './amp-story-auto-ads'; import './amp-story-grid-layer'; import './amp-story-page'; +import { + Action, + AmpStoryStoreService, + StateProperty, +} from './amp-story-store-service'; import {ActionTrust} from '../../../src/action-trust'; import {AmpStoryAnalytics} from './analytics'; import {AmpStoryBackground} from './background'; import {AmpStoryHint} from './amp-story-hint'; -import {AmpStoryStoreService, StateProperty} from './amp-story-store-service'; import {AmpStoryVariableService} from './variable-service'; import {Bookend} from './bookend'; import {CSS} from '../../../build/amp-story-0.1.css'; @@ -257,7 +261,7 @@ export class AmpStory extends AMP.BaseElement { /** @private {!NavigationState} */ this.navigationState_ = - new NavigationState(element, () => this.hasBookend_()); + new NavigationState(this.win, () => this.hasBookend_()); /** @const @private {!../../../src/service/vsync-impl.Vsync} */ this.vsync_ = this.getVsync(); @@ -397,18 +401,6 @@ export class AmpStory extends AMP.BaseElement { this.previous_(); }); - this.element.addEventListener(EventType.SHOW_BOOKEND, () => { - this.hasBookend_().then(hasBookend => { - if (hasBookend) { - this.showBookend_(); - } - }); - }); - - this.element.addEventListener(EventType.CLOSE_BOOKEND, () => { - this.hideBookend_(); - }); - this.element.addEventListener(EventType.MUTE, () => { this.mute_(); }); @@ -426,7 +418,7 @@ export class AmpStory extends AMP.BaseElement { }); this.element.addEventListener(EventType.SWITCH_PAGE, e => { - if (this.bookend_.isActive()) { + if (this.storeService_.get(StateProperty.BOOKEND_STATE)) { // Disallow switching pages while the bookend is active. return; } @@ -464,6 +456,10 @@ export class AmpStory extends AMP.BaseElement { this.performTapNavigation_(direction); }); + this.storeService_.subscribe(StateProperty.BOOKEND_STATE, isActive => { + this.onBookendStateUpdate_(isActive); + }); + this.win.document.addEventListener('keydown', e => { this.onKeyDown_(e); }, true); @@ -553,7 +549,7 @@ export class AmpStory extends AMP.BaseElement { /** @private */ buildPaginationButtons_() { - this.paginationButtons_ = PaginationButtons.create(this.win.document); + this.paginationButtons_ = PaginationButtons.create(this.win); this.paginationButtons_.attach(this.element); @@ -794,7 +790,11 @@ export class AmpStory extends AMP.BaseElement { activePage !== lastPage) { activePage.next(opt_isAutomaticAdvance); } else { - dispatch(this.element, EventType.SHOW_BOOKEND); + this.hasBookend_().then(hasBookend => { + if (hasBookend) { + this.showBookend_(); + } + }); } } @@ -1099,20 +1099,8 @@ export class AmpStory extends AMP.BaseElement { * @private */ showBookend_() { - if (this.bookend_.isActive()) { - return; - } - this.buildBookend_().then(() => { - this.systemLayer_.hideDeveloperLog(); - - this.activePage_.pauseCallback(); - - this.toggleElementsOnBookend_(/* display */ false); - - this.element.classList.add('i-amphtml-story-bookend-active'); - - this.bookend_.show(); + this.storeService_.dispatch(Action.TOGGLE_BOOKEND, true); }); } @@ -1122,25 +1110,35 @@ export class AmpStory extends AMP.BaseElement { * @private */ hideBookend_() { - if (!this.bookend_.isActive()) { - return; - } + this.storeService_.dispatch(Action.TOGGLE_BOOKEND, false); + } - this.activePage_.resumeCallback(); - this.toggleElementsOnBookend_(/* display */ true); + /** + * @param {boolean} isActive + * @private + */ + onBookendStateUpdate_(isActive) { + this.toggleElementsOnBookend_(/* display */ isActive); + this.element.classList.toggle('i-amphtml-story-bookend-active', isActive); - this.element.classList.remove('i-amphtml-story-bookend-active'); + if (isActive) { + this.systemLayer_.hideDeveloperLog(); + this.activePage_.pauseCallback(); + } - this.bookend_.hide(); + if (!isActive) { + this.activePage_.resumeCallback(); + } } + /** * Toggle content when bookend is opened/closed. - * @param {boolean} display + * @param {boolean} isActive * @private */ - toggleElementsOnBookend_(display) { + toggleElementsOnBookend_(isActive) { if (!this.isDesktop_()) { return; } @@ -1149,13 +1147,13 @@ export class AmpStory extends AMP.BaseElement { HIDE_ON_BOOKEND_SELECTOR); Array.prototype.forEach.call(elements, el => { - if (display) { - resetStyles(el, ['opacity', 'transition']); - } else { + if (isActive) { setImportantStyles(el, { opacity: 0, transition: 'opacity 0.3s', }); + } else { + resetStyles(el, ['opacity', 'transition']); } }); } @@ -1330,7 +1328,7 @@ export class AmpStory extends AMP.BaseElement { return Promise.resolve(true); } return this.loadBookendConfig_().then(config => - config && config.relatedArticles && config.relatedArticles.length); + !!(config && config.relatedArticles && config.relatedArticles.length)); } @@ -1358,26 +1356,6 @@ export class AmpStory extends AMP.BaseElement { } - /** - * @param {!Element} el - * @return {boolean} - * @private - */ - isBookend_(el) { - return this.bookend_.isBuilt() && el === this.bookend_.getRoot(); - } - - - /** - * @param {!Element} el - * @return {boolean} - * @private - */ - isTopBar_(el) { - return !!this.topBar_ && this.topBar_.contains(el); - } - - /** * @param {string} id The ID of the page whose index should be retrieved. * @return {number} The index of the page. @@ -1588,9 +1566,7 @@ export class AmpStory extends AMP.BaseElement { /** @private */ replay_() { if (this.bookend_.isActive()) { - // Dispaching event instead of calling method directly so that all - // listeners can respond. - dispatch(this.element, EventType.CLOSE_BOOKEND); + this.hideBookend_(); } this.switchTo_(dev().assertElement(this.pages_[0].element).id); } diff --git a/extensions/amp-story/0.1/bookend.js b/extensions/amp-story/0.1/bookend.js index 68d5c92fca66..7d4cf32ab9b8 100644 --- a/extensions/amp-story/0.1/bookend.js +++ b/extensions/amp-story/0.1/bookend.js @@ -13,6 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import {Action, StateProperty} from './amp-story-store-service'; import {EventType, dispatch} from './events'; import {KeyCodes} from '../../../src/utils/key-codes'; import {ScrollableShareWidget} from './share'; @@ -211,7 +212,10 @@ export class Bookend { this.closeBtn_ = null; /** @private {!ScrollableShareWidget} */ - this.shareWidget_ = ScrollableShareWidget.create(win); + this.shareWidget_ = ScrollableShareWidget.create(this.win_); + + /** @private @const {!./amp-story-store-service.AmpStoryStoreService} */ + this.storeService_ = Services.storyStoreService(this.win_); } /** @@ -254,9 +258,12 @@ export class Bookend { } if (e.keyCode == KeyCodes.ESCAPE) { e.preventDefault(); - this.dispatchClose_(); + this.close_(); } }); + + this.storeService_.subscribe( + StateProperty.BOOKEND_STATE, isActive => this.toggle_(isActive)); } /** @return {boolean} */ @@ -282,13 +289,15 @@ export class Bookend { maybeClose_(e) { if (this.elementOutsideUsableArea_(dev().assertElement(e.target))) { e.stopPropagation(); - this.dispatchClose_(); + this.close_(); } } - /** @private */ - dispatchClose_() { - dispatch(this.getRoot(), EventType.CLOSE_BOOKEND, /* opt_bubbles */ true); + /** + * Closes the bookend. + */ + close_() { + this.storeService_.dispatch(Action.TOGGLE_BOOKEND, false); } /** @@ -319,16 +328,6 @@ export class Bookend { }, {}); } - /** Hides bookend with a transition. */ - hide() { - this.toggle_(false); - } - - /** Shows bookend with a transition. */ - show() { - this.toggle_(true); - } - /** * @param {boolean} show * @private diff --git a/extensions/amp-story/0.1/events.js b/extensions/amp-story/0.1/events.js index 902263c3a4f9..b810b42b9c35 100644 --- a/extensions/amp-story/0.1/events.js +++ b/extensions/amp-story/0.1/events.js @@ -19,12 +19,6 @@ import {createCustomEvent} from '../../../src/event-helper'; /** @const {!Object} */ export const EventType = { - // Triggered when the bookend should be opened - SHOW_BOOKEND: 'ampstory:showbookend', - - // Triggered when the user clicks the close bookend button - CLOSE_BOOKEND: 'ampstory:closebookend', - // Triggered when the user mutes the story MUTE: 'ampstory:mute', diff --git a/extensions/amp-story/0.1/navigation-state.js b/extensions/amp-story/0.1/navigation-state.js index 97cb3046bbc1..b69bdb6f5c00 100644 --- a/extensions/amp-story/0.1/navigation-state.js +++ b/extensions/amp-story/0.1/navigation-state.js @@ -13,8 +13,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import {EventType} from './events'; import {Observable} from '../../../src/observable'; +import {Services} from '../../../src/services'; +import {StateProperty} from './amp-story-store-service'; /** @@ -38,35 +39,44 @@ export let StateChangeEventDef; */ export class NavigationState { /** - * @param {!Element} storyElement + * @param {!Window} win * @param {function():Promise} hasBookend */ - constructor(storyElement, hasBookend) { + constructor(win, hasBookend) { /** @private @const {!function():Promise} */ this.hasBookend_ = hasBookend; /** @private {!Observable} */ this.observable_ = new Observable(); - this.attachEvents_(storyElement); + /** @private @const {!Window} */ + this.win_ = win; + + /** @private @const {!./amp-story-store-service.AmpStoryStoreService} */ + this.storeService_ = Services.storyStoreService(this.win_); + + this.initializeListeners_(); } /** - * @param {!Element} storyElement * @private */ - attachEvents_(storyElement) { - storyElement.addEventListener(EventType.SHOW_BOOKEND, () => { - this.fire_(StateChangeType.BOOKEND_ENTER); - this.fire_(StateChangeType.END); - }); - - storyElement.addEventListener(EventType.CLOSE_BOOKEND, () => { - this.fire_(StateChangeType.BOOKEND_EXIT); + initializeListeners_() { + this.storeService_.subscribe(StateProperty.BOOKEND_STATE, isActive => { + if (isActive) { + this.fire_(StateChangeType.BOOKEND_ENTER); + this.fire_(StateChangeType.END); + } + + if (!isActive) { + this.fire_(StateChangeType.BOOKEND_EXIT); + } }); } - /** @param {function(!StateChangeEventDef):void} stateChangeFn */ + /** + * @param {function(!StateChangeEventDef):void} stateChangeFn + */ observe(stateChangeFn) { this.observable_.add(stateChangeFn); } @@ -74,14 +84,14 @@ export class NavigationState { /** * @param {number} pageIndex * @param {number} totalPages - * @param {string=} opt_pageId + * @param {string=} pageId */ // TODO(alanorozco): pass whether change was automatic or on user action - updateActivePage(pageIndex, totalPages, opt_pageId) { + updateActivePage(pageIndex, totalPages, pageId = undefined) { const changeValue = {pageIndex, totalPages}; - if (opt_pageId) { - changeValue.pageId = opt_pageId; + if (pageId) { + changeValue.pageId = pageId; } this.fire_(StateChangeType.ACTIVE_PAGE, changeValue); diff --git a/extensions/amp-story/0.1/pagination-buttons.js b/extensions/amp-story/0.1/pagination-buttons.js index d8a9fb980b52..2bd9bce6042f 100644 --- a/extensions/amp-story/0.1/pagination-buttons.js +++ b/extensions/amp-story/0.1/pagination-buttons.js @@ -13,7 +13,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import {Action} from './amp-story-store-service'; import {EventType, dispatch} from './events'; +import {Services} from '../../../src/services'; import {StateChangeType} from './navigation-state'; import {dev} from '../../../src/log'; import {dict} from './../../../src/utils/object'; @@ -26,15 +28,16 @@ let ButtonStateDef; /** @const {!Object} */ const BackButtonStates = { + CLOSE_BOOKEND: { + className: 'i-amphtml-story-back-close-bookend', + action: Action.TOGGLE_BOOKEND, + data: false, + }, HIDDEN: {className: 'i-amphtml-story-button-hidden'}, PREVIOUS_PAGE: { className: 'i-amphtml-story-back-prev', triggers: EventType.PREVIOUS_PAGE, }, - CLOSE_BOOKEND: { - className: 'i-amphtml-story-back-close-bookend', - triggers: EventType.CLOSE_BOOKEND, - }, }; @@ -44,14 +47,15 @@ const ForwardButtonStates = { className: 'i-amphtml-story-fwd-next', triggers: EventType.NEXT_PAGE, }, - SHOW_BOOKEND: { - className: 'i-amphtml-story-fwd-more', - triggers: EventType.SHOW_BOOKEND, - }, REPLAY: { className: 'i-amphtml-story-fwd-replay', triggers: EventType.REPLAY, }, + SHOW_BOOKEND: { + className: 'i-amphtml-story-fwd-more', + action: Action.TOGGLE_BOOKEND, + data: true, + }, }; @@ -91,8 +95,9 @@ class PaginationButton { /** * @param {!Document} doc * @param {!ButtonStateDef} initialState + * @param {!./amp-story-store-service.AmpStoryStoreService} storeService */ - constructor(doc, initialState) { + constructor(doc, initialState, storeService) { /** @private {!ButtonStateDef} */ this.state_ = initialState; @@ -102,6 +107,9 @@ class PaginationButton { this.element.classList.add(initialState.className); this.element.addEventListener('click', e => this.onClick_(e)); + + /** @private @const {!./amp-story-store-service.AmpStoryStoreService} */ + this.storeService_ = storeService; } /** @param {!ButtonStateDef} state */ @@ -119,37 +127,45 @@ class PaginationButton { * @private */ onClick_(e) { - if (!this.state_.triggers) { + e.preventDefault(); + if (this.state_.triggers) { + dispatch(this.element, dev().assert(this.state_.triggers), + /* opt_bubbles */ true); + return; + } + if (this.state_.action) { + this.storeService_.dispatch(this.state_.action, this.state_.data); return; } - e.preventDefault(); - dispatch(this.element, dev().assert(this.state_.triggers), - /* opt_bubbles */ true); } } /** Pagination buttons layer. */ export class PaginationButtons { - /** @param {!Document} doc */ - constructor(doc) { + /** @param {!Window} win */ + constructor(win) { + const doc = win.document; + const storeService = Services.storyStoreService(win); + /** @private @const {!PaginationButton} */ this.forwardButton_ = - new PaginationButton(doc, ForwardButtonStates.NEXT_PAGE); + new PaginationButton(doc, ForwardButtonStates.NEXT_PAGE, storeService); /** @private @const {!PaginationButton} */ - this.backButton_ = new PaginationButton(doc, BackButtonStates.HIDDEN); + this.backButton_ = + new PaginationButton(doc, BackButtonStates.HIDDEN, storeService); this.forwardButton_.element.classList.add('next-container'); this.backButton_.element.classList.add('prev-container'); } /** - * @param {!Document} doc + * @param {!Window} win * @return {!PaginationButtons} */ - static create(doc) { - return new PaginationButtons(doc); + static create(win) { + return new PaginationButtons(win); } /** @param {!Element} element */ diff --git a/extensions/amp-story/0.1/test/test-amp-story-store-service.js b/extensions/amp-story/0.1/test/test-amp-story-store-service.js index 78fe5515ddfd..172c13eb330a 100644 --- a/extensions/amp-story/0.1/test/test-amp-story-store-service.js +++ b/extensions/amp-story/0.1/test/test-amp-story-store-service.js @@ -63,3 +63,28 @@ describes.fakeWin('amp-story-store-service embed mode', {}, env => { expect(storeService.get(StateProperty.CAN_SHOW_BOOKEND)).to.be.false; }); }); + +describes.fakeWin('amp-story-store-service actions', {}, env => { + let storeService; + + beforeEach(() => { + // Making sure we always get a new instance to isolate each test. + storeService = new AmpStoryStoreService(env.win); + }); + + it('should toggle the bookend', () => { + const listenerSpy = sandbox.spy(); + storeService.subscribe(StateProperty.BOOKEND_STATE, listenerSpy); + storeService.dispatch(Action.TOGGLE_BOOKEND, true); + expect(listenerSpy).to.have.been.calledOnce; + expect(listenerSpy).to.have.been.calledWith(true); + }); + + it('should not toggle the bookend if embed mode disables it', () => { + const listenerSpy = sandbox.spy(); + storeService.state_[StateProperty.CAN_SHOW_BOOKEND] = false; + storeService.subscribe(StateProperty.BOOKEND_STATE, listenerSpy); + storeService.dispatch(Action.TOGGLE_BOOKEND, true); + expect(listenerSpy).to.have.callCount(0); + }); +}); diff --git a/extensions/amp-story/0.1/test/test-navigation-state.js b/extensions/amp-story/0.1/test/test-navigation-state.js index c062036da75e..4a15a16f03bf 100644 --- a/extensions/amp-story/0.1/test/test-navigation-state.js +++ b/extensions/amp-story/0.1/test/test-navigation-state.js @@ -13,23 +13,25 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import {EventType, dispatch} from '../events'; +import {Action, AmpStoryStoreService} from '../amp-story-store-service'; import {NavigationState, StateChangeType} from '../navigation-state'; +import {registerServiceBuilder} from '../../../../src/service'; describes.fakeWin('amp-story navigation state', {ampdoc: 'none'}, env => { let navigationState; - let pageElement; let hasBookend = false; + let storeService; function createObserver() { return sandbox.spy(); } beforeEach(() => { + storeService = new AmpStoryStoreService(env.win); + registerServiceBuilder(env.win, 'story-store', () => storeService); hasBookend = false; - pageElement = env.win.document.createElement('div'); - navigationState = new NavigationState(pageElement, + navigationState = new NavigationState(env.win, // Not using `Promise.resolve` since we need synchronicity. () => ({then(fn) { fn(hasBookend); }})); }); @@ -111,12 +113,12 @@ describes.fakeWin('amp-story navigation state', {ampdoc: 'none'}, env => { navigationState.observe(event => observer(event.type)); - dispatch(pageElement, EventType.SHOW_BOOKEND); + storeService.dispatch(Action.TOGGLE_BOOKEND, true); expect(observer).to.have.been.calledWith(StateChangeType.BOOKEND_ENTER); expect(observer).to.have.been.calledWith(StateChangeType.END); - dispatch(pageElement, EventType.CLOSE_BOOKEND); + storeService.dispatch(Action.TOGGLE_BOOKEND, false); expect(observer).to.have.been.calledWith(StateChangeType.BOOKEND_EXIT); });