From ac5e6baced0a660f1e4a13ed2318cd8461288dd3 Mon Sep 17 00:00:00 2001 From: Ward Bell Date: Mon, 24 Apr 2017 14:34:31 -0700 Subject: [PATCH] fix(aio): AppComponent should scroll only once when location changes --- aio/src/app/app.component.spec.ts | 52 ++++++++++++++++++++------ aio/src/app/app.component.ts | 14 +++++-- aio/src/app/shared/location.service.ts | 3 +- aio/src/testing/location.service.ts | 7 +++- 4 files changed, 60 insertions(+), 16 deletions(-) diff --git a/aio/src/app/app.component.spec.ts b/aio/src/app/app.component.spec.ts index 144de9ab68..c58d53dcf2 100644 --- a/aio/src/app/app.component.spec.ts +++ b/aio/src/app/app.component.spec.ts @@ -265,19 +265,49 @@ describe('AppComponent', () => { }); }); - describe('autoScrolling', () => { - it('should AutoScrollService.scroll when the url changes', () => { - const scrollService: AutoScrollService = fixture.debugElement.injector.get(AutoScrollService); - spyOn(scrollService, 'scroll'); - locationService.go('some/url#fragment'); - expect(scrollService.scroll).toHaveBeenCalledWith(); + describe('autoScrolling with AutoScrollService', () => { + let scrollService: AutoScrollService; + let scrollSpy: jasmine.Spy; + + beforeEach(() => { + scrollService = fixture.debugElement.injector.get(AutoScrollService); + scrollSpy = spyOn(scrollService, 'scroll'); }); - it('should be called when a document has been rendered', () => { - const scrollService: AutoScrollService = fixture.debugElement.injector.get(AutoScrollService); - spyOn(scrollService, 'scroll'); + it('should not scroll immediately when the docId (path) changes', () => { + locationService.go('guide/pipes'); + // deliberately not calling `fixture.detectChanges` because don't want `onDocRendered` + expect(scrollSpy).not.toHaveBeenCalled(); + }); + + it('should scroll when just the hash changes (# alone)', () => { + locationService.go('guide/pipes'); + locationService.go('guide/pipes#somewhere'); + expect(scrollSpy).toHaveBeenCalled(); + }); + + it('should scroll when just the hash changes (/#)', () => { + locationService.go('guide/pipes'); + locationService.go('guide/pipes/#somewhere'); + expect(scrollSpy).toHaveBeenCalled(); + }); + + it('should scroll again when nav to the same hash twice in succession', () => { + locationService.go('guide/pipes'); + locationService.go('guide/pipes#somewhere'); + locationService.go('guide/pipes#somewhere'); + expect(scrollSpy.calls.count()).toBe(2); + }); + + it('should scroll when call onDocRendered directly', () => { component.onDocRendered(); - expect(scrollService.scroll).toHaveBeenCalledWith(); + expect(scrollSpy).toHaveBeenCalled(); + }); + + it('should scroll (via onDocRendered) when finish navigating to a new doc', () => { + locationService.go('guide/pipes'); + fixture.detectChanges(); // triggers the event that calls onDocRendered + expect(scrollSpy).toHaveBeenCalled(); }); }); @@ -464,7 +494,7 @@ class TestHttp { const match = /content\/docs\/(.+)\.json/.exec(url); const id = match[1]; const title = id.split('/').pop().replace(/^([a-z])/, (_, letter) => letter.toUpperCase()); - const contents = `

${title} Doc

`; + const contents = `

${title} Doc

Some heading

`; data = { id, title, contents }; if (id === 'no-title') { data.title = ''; diff --git a/aio/src/app/app.component.ts b/aio/src/app/app.component.ts index 71669c39aa..886177ae28 100644 --- a/aio/src/app/app.component.ts +++ b/aio/src/app/app.component.ts @@ -21,6 +21,7 @@ const sideNavView = 'SideNav'; export class AppComponent implements OnInit { currentNode: CurrentNode; + currentPath: string; dtOn = false; pageId: string; currentDocument: DocumentContents; @@ -76,8 +77,15 @@ export class AppComponent implements OnInit { this.setPageId(doc.id); }); - // scroll even if only the hash fragment changed - this.locationService.currentUrl.subscribe(() => this.autoScroll()); + this.locationService.currentPath.subscribe(path => { + if (this.currentPath && path === this.currentPath) { + // scroll only if on same page (most likely a change to the hash) + this.autoScroll(); + } else { + // don't scroll; leave that to `onDocRendered` + this.currentPath = path; + } + }); this.navigationService.currentNode.subscribe(currentNode => { this.currentNode = currentNode; @@ -100,7 +108,7 @@ export class AppComponent implements OnInit { this.swUpdateNotifications.enable(); } - // Scroll to the anchor in the hash fragment. + // Scroll to the anchor in the hash fragment or top of doc. autoScroll() { this.autoScrollService.scroll(); } diff --git a/aio/src/app/shared/location.service.ts b/aio/src/app/shared/location.service.ts index 8e41d54a4e..50c3668996 100644 --- a/aio/src/app/shared/location.service.ts +++ b/aio/src/app/shared/location.service.ts @@ -17,8 +17,9 @@ export class LocationService { .map(url => this.stripSlashes(url)) .do(url => this.gaService.locationChanged(url)) .publishReplay(1); + currentPath = this.currentUrl - .map(url => url.match(/[^?#]*/)[0]); // strip off query and hash + .map(url => url.match(/[^?#]*/)[0]); // strip query and hash constructor( private gaService: GaService, diff --git a/aio/src/testing/location.service.ts b/aio/src/testing/location.service.ts index ae1df87038..ad8bb2d329 100644 --- a/aio/src/testing/location.service.ts +++ b/aio/src/testing/location.service.ts @@ -2,7 +2,8 @@ import { BehaviorSubject } from 'rxjs/BehaviorSubject'; export class MockLocationService { urlSubject = new BehaviorSubject(this.initialUrl); - currentUrl = this.urlSubject.asObservable(); + currentUrl = this.urlSubject.asObservable().map(url => this.stripSlashes(url)); + // strip off query and hash currentPath = this.currentUrl.map(url => url.match(/[^?#]*/)[0]); search = jasmine.createSpy('search').and.returnValue({}); setSearch = jasmine.createSpy('setSearch'); @@ -12,5 +13,9 @@ export class MockLocationService { .and.returnValue(false); // prevent click from causing a browser navigation constructor(private initialUrl) {} + + private stripSlashes(url: string) { + return url.replace(/^\/+/, '').replace(/\/+(\?|#|$)/, '$1'); + } }