Skip to content

Commit aea52cd

Browse files
authored
🚮 Remove navigation state (ampproject#23110)
* remove navigation state * fix types * fix tests * remove comment about using pageId, check for a pageId instead; use callToInitialize * pageId
1 parent 84d0bad commit aea52cd

File tree

10 files changed

+109
-427
lines changed

10 files changed

+109
-427
lines changed

‎build-system/dep-check-config.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,6 @@ exports.rules = [
255255
'extensions/amp-story/1.0/animation.js->extensions/amp-animation/0.1/web-animation-types.js',
256256
// Story ads
257257
'extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js->extensions/amp-story/1.0/amp-story-store-service.js',
258-
'extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js->extensions/amp-story/1.0/navigation-state.js',
259258
// TODO(ccordry): remove this after createShadowRootWithStyle is moved to src
260259
'extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js->extensions/amp-story/1.0/utils.js',
261260

‎extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js‎

Lines changed: 11 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,6 @@ import {
2222
createPseudoLocale,
2323
} from '../../../src/localized-strings';
2424
import {Services} from '../../../src/services';
25-
import {
26-
StateChangeEventDef,
27-
StateChangeType,
28-
} from '../../amp-story/1.0/navigation-state';
2925
import {
3026
StateProperty,
3127
UIType,
@@ -195,9 +191,6 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
195191
/** @private {?../../amp-story/1.0/amp-story.AmpStory} */
196192
this.ampStory_ = null;
197193

198-
/** @private {?../../amp-story/1.0/navigation-state.NavigationState} */
199-
this.navigationState_ = null;
200-
201194
/** @private {number} */
202195
this.uniquePagesCount_ = 0;
203196

@@ -327,11 +320,6 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
327320
return Promise.resolve();
328321
}
329322

330-
// TODO(ccordry): Move all this to use store service. We would like to
331-
// eventually deprecate navigationState.
332-
this.navigationState_ = this.ampStory_.getNavigationState();
333-
this.navigationState_.observe(this.handleStateChange_.bind(this));
334-
335323
return this.ampStory_
336324
.signals()
337325
.whenSignal(CommonSignals.INI_LOAD)
@@ -377,6 +365,17 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
377365
},
378366
true /** callToInitialize */
379367
);
368+
369+
this.storeService_.subscribe(StateProperty.CURRENT_PAGE_ID, pageId => {
370+
const pageIndex = this.storeService_.get(
371+
StateProperty.CURRENT_PAGE_INDEX
372+
);
373+
374+
this.handleActivePageChange_(
375+
dev().assertNumber(pageIndex),
376+
dev().assertString(pageId)
377+
);
378+
});
380379
}
381380

382381
/**
@@ -710,22 +709,6 @@ export class AmpStoryAutoAds extends AMP.BaseElement {
710709
return true;
711710
}
712711

713-
/**
714-
* @param {!StateChangeEventDef} stateChangeEvent
715-
* @private
716-
*/
717-
handleStateChange_(stateChangeEvent) {
718-
switch (stateChangeEvent.type) {
719-
case StateChangeType.ACTIVE_PAGE:
720-
const {pageIndex, pageId} = stateChangeEvent.value;
721-
this.handleActivePageChange_(
722-
dev().assertNumber(pageIndex),
723-
dev().assertString(pageId)
724-
);
725-
break;
726-
}
727-
}
728-
729712
/**
730713
* @param {number} pageIndex
731714
* @param {string} pageId

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

Lines changed: 0 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,6 @@ import {Layout} from '../../../src/layout';
7171
import {LiveStoryManager} from './live-story-manager';
7272
import {LocalizationService} from '../../../src/service/localization';
7373
import {MediaPool, MediaType} from './media-pool';
74-
import {NavigationState} from './navigation-state';
7574
import {PaginationButtons} from './pagination-buttons';
7675
import {Services} from '../../../src/services';
7776
import {ShareMenu} from './amp-story-share-menu';
@@ -245,12 +244,6 @@ export class AmpStory extends AMP.BaseElement {
245244
this.storeService_.dispatch(Action.TOGGLE_RTL, true);
246245
}
247246

248-
// TODO(#19768): Avoid passing a private function here.
249-
/** @private {!NavigationState} */
250-
this.navigationState_ = new NavigationState(this.win, () =>
251-
this.hasBookend_()
252-
);
253-
254247
/** @private {!./story-analytics.StoryAnalyticsService} */
255248
this.analyticsService_ = getAnalyticsService(this.win, this.element);
256249

@@ -434,11 +427,6 @@ export class AmpStory extends AMP.BaseElement {
434427
this.storeService_.dispatch(Action.TOGGLE_CAN_SHOW_BOOKEND, false);
435428
}
436429

437-
this.navigationState_.observe(stateChangeEvent => {
438-
this.variableService_.onNavigationStateChange(stateChangeEvent);
439-
this.analyticsService_.onNavigationStateChange(stateChangeEvent);
440-
});
441-
442430
// Removes title in order to prevent incorrect titles appearing on link
443431
// hover. (See 17654)
444432
this.element.removeAttribute('title');
@@ -1449,16 +1437,6 @@ export class AmpStory extends AMP.BaseElement {
14491437
}
14501438
}
14511439

1452-
const oldPageId = oldPage ? oldPage.element.id : null;
1453-
// TODO(alanorozco): check if autoplay
1454-
this.navigationState_.updateActivePage(
1455-
pageIndex,
1456-
this.getPageCount(),
1457-
targetPage.element.id,
1458-
oldPageId,
1459-
targetPage.getNextPageId() === null /* isFinalPage */
1460-
);
1461-
14621440
// If first navigation.
14631441
if (!oldPage) {
14641442
this.registerAndPreloadBackgroundAudio_();
@@ -2661,11 +2639,6 @@ export class AmpStory extends AMP.BaseElement {
26612639
});
26622640
}
26632641

2664-
/** @return {!NavigationState} */
2665-
getNavigationState() {
2666-
return this.navigationState_;
2667-
}
2668-
26692642
/**
26702643
* Add page to back of pages_ array
26712644
* @param {!./amp-story-page.AmpStoryPage} page

‎extensions/amp-story/1.0/navigation-state.js‎

Lines changed: 0 additions & 117 deletions
This file was deleted.

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

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* limitations under the License.
1515
*/
1616
import {Services} from '../../../src/services';
17-
import {StateChangeType} from './navigation-state';
17+
import {StateProperty, getStoreService} from './amp-story-store-service';
1818
import {getVariableService} from './variable-service';
1919
import {registerServiceBuilder} from '../../../src/service';
2020
import {triggerAnalyticsEvent} from '../../../src/analytics';
@@ -76,26 +76,40 @@ export class StoryAnalyticsService {
7676

7777
/** @const @private {!./variable-service.AmpStoryVariableService} */
7878
this.variableService_ = getVariableService(win);
79+
80+
/** @private @const {!./amp-story-store-service.AmpStoryStoreService} */
81+
this.storeService_ = getStoreService(win);
82+
83+
this.initializeListeners_();
7984
}
8085

81-
/**
82-
* @param {!./navigation-state.StateChangeEventDef} stateChangeEvent
83-
*/
84-
onNavigationStateChange(stateChangeEvent) {
85-
switch (stateChangeEvent.type) {
86-
case StateChangeType.ACTIVE_PAGE:
86+
/** @private */
87+
initializeListeners_() {
88+
this.storeService_.subscribe(StateProperty.BOOKEND_STATE, isActive => {
89+
this.triggerEvent(
90+
isActive ? AnalyticsEvent.BOOKEND_ENTER : AnalyticsEvent.BOOKEND_EXIT
91+
);
92+
});
93+
94+
this.storeService_.subscribe(
95+
StateProperty.CURRENT_PAGE_ID,
96+
pageId => {
97+
if (!pageId) {
98+
return;
99+
}
100+
87101
this.triggerEvent(AnalyticsEvent.PAGE_VISIBLE);
88-
break;
89-
case StateChangeType.BOOKEND_ENTER:
90-
this.triggerEvent(AnalyticsEvent.BOOKEND_ENTER);
91-
break;
92-
case StateChangeType.BOOKEND_EXIT:
93-
this.triggerEvent(AnalyticsEvent.BOOKEND_EXIT);
94-
break;
95-
case StateChangeType.LAST_PAGE:
96-
this.triggerEvent(AnalyticsEvent.LAST_PAGE_VISIBLE);
97-
break;
98-
}
102+
103+
const pageIds = this.storeService_.get(StateProperty.PAGE_IDS);
104+
const pageIndex = this.storeService_.get(
105+
StateProperty.CURRENT_PAGE_INDEX
106+
);
107+
if (pageIndex === pageIds.length - 1) {
108+
this.triggerEvent(AnalyticsEvent.LAST_PAGE_VISIBLE);
109+
}
110+
},
111+
true /* callToInitialize */
112+
);
99113
}
100114

101115
/**

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

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -165,24 +165,6 @@ describes.realWin(
165165
});
166166
});
167167

168-
it('should update the navigation state when built', () => {
169-
const firstPageId = 'cover';
170-
const pageCount = 2;
171-
createPages(story.element, pageCount, [firstPageId, 'page-1']);
172-
const updateActivePageStub = sandbox.stub(
173-
story.navigationState_,
174-
'updateActivePage'
175-
);
176-
177-
return story.layoutCallback().then(() => {
178-
expect(updateActivePageStub).to.have.been.calledWith(
179-
0,
180-
pageCount,
181-
firstPageId
182-
);
183-
});
184-
});
185-
186168
it('should remove text child nodes when built', () => {
187169
createPages(story.element, 1, ['cover']);
188170
const textToRemove = 'this should be removed';
@@ -321,10 +303,7 @@ describes.realWin(
321303
createPages(story.element, 2, ['cover', 'page-1']);
322304

323305
return story.layoutCallback().then(() => {
324-
const paginationButtonsStub = {
325-
attach: sandbox.spy(),
326-
onNavigationStateChange: sandbox.spy(),
327-
};
306+
const paginationButtonsStub = {attach: sandbox.spy()};
328307
sandbox
329308
.stub(PaginationButtons, 'create')
330309
.returns(paginationButtonsStub);

0 commit comments

Comments
 (0)