refactor(aio): rename auto-scroll -> scroll and refactor

Refactor inspired by @gkalpak feedback on PR #16619

aio/src/app/app.component.html

@@ -10,7 +10,6 @@
 </md-toolbar>
 <aio-search-results #searchResults></aio-search-results>
 
-<a id="top-of-page"></a>
 <md-sidenav-container class="sidenav-container" [class.starting]="isStarting" role="main">
 
   <md-sidenav [ngClass]="{'collapsed': !isSideBySide }" #sidenav class="sidenav" [opened]="isOpened" [mode]="mode">
@@ -25,6 +24,7 @@
   </md-sidenav>
 
   <section class="sidenav-content" [id]="pageId" role="content">
+    <a id="top-of-page"></a>
     <aio-doc-viewer [doc]="currentDocument" (docRendered)="onDocRendered()"></aio-doc-viewer>
     <aio-dt [on]="dtOn" [(doc)]="currentDocument"></aio-dt>
   </section>

aio/src/app/app.component.spec.ts

@@ -10,21 +10,21 @@ import { of } from 'rxjs/observable/of';
 
 import { AppComponent } from './app.component';
 import { AppModule } from './app.module';
-import { TocComponent } from 'app/embedded/toc/toc.component';
 import { DocViewerComponent } from 'app/layout/doc-viewer/doc-viewer.component';
-import { NavigationNode } from 'app/navigation/navigation.service';
-import { SearchService } from 'app/search/search.service';
-import { SearchBoxComponent } from 'app/search/search-box/search-box.component';
-import { SearchResultsComponent } from 'app/search/search-results/search-results.component';
-import { AutoScrollService } from 'app/shared/auto-scroll.service';
 import { GaService } from 'app/shared/ga.service';
 import { LocationService } from 'app/shared/location.service';
 import { Logger } from 'app/shared/logger.service';
-import { SwUpdateNotificationsService } from 'app/sw-updates/sw-update-notifications.service';
 import { MockLocationService } from 'testing/location.service';
 import { MockLogger } from 'testing/logger.service';
 import { MockSearchService } from 'testing/search.service';
 import { MockSwUpdateNotificationsService } from 'testing/sw-update-notifications.service';
+import { NavigationNode } from 'app/navigation/navigation.service';
+import { ScrollService } from 'app/shared/scroll.service';
+import { SearchBoxComponent } from 'app/search/search-box/search-box.component';
+import { SearchResultsComponent } from 'app/search/search-results/search-results.component';
+import { SearchService } from 'app/search/search.service';
+import { SwUpdateNotificationsService } from 'app/sw-updates/sw-update-notifications.service';
+import { TocComponent } from 'app/embedded/toc/toc.component';
 
 describe('AppComponent', () => {
   let component: AppComponent;
@@ -319,12 +319,12 @@ describe('AppComponent', () => {
     });
   });
 
-  describe('autoScrolling with AutoScrollService', () => {
+  describe('auto-scrolling', () => {
-    let scrollService: AutoScrollService;
+    let scrollService: ScrollService;
     let scrollSpy: jasmine.Spy;
 
     beforeEach(() => {
-      scrollService = fixture.debugElement.injector.get(AutoScrollService);
+      scrollService = fixture.debugElement.injector.get(ScrollService);
       scrollSpy = spyOn(scrollService, 'scroll');
     });
 

aio/src/app/app.component.ts

@@ -2,12 +2,12 @@ import { Component, ElementRef, HostBinding, HostListener, OnInit,
          QueryList, ViewChild, ViewChildren } from '@angular/core';
 import { MdSidenav } from '@angular/material';
 
-import { AutoScrollService } from 'app/shared/auto-scroll.service';
 import { CurrentNode, NavigationService, NavigationViews, NavigationNode, VersionInfo } from 'app/navigation/navigation.service';
 import { DocumentService, DocumentContents } from 'app/documents/document.service';
 import { DocViewerComponent } from 'app/layout/doc-viewer/doc-viewer.component';
 import { LocationService } from 'app/shared/location.service';
 import { NavMenuComponent } from 'app/layout/nav-menu/nav-menu.component';
+import { ScrollService } from 'app/shared/scroll.service';
 import { SearchResultsComponent } from 'app/search/search-results/search-results.component';
 import { SwUpdateNotificationsService } from 'app/sw-updates/sw-update-notifications.service';
 
@@ -84,11 +84,11 @@ export class AppComponent implements OnInit {
   sidenav: MdSidenav;
 
   constructor(
-    private autoScrollService: AutoScrollService,
     private documentService: DocumentService,
     private hostElement: ElementRef,
     private locationService: LocationService,
     private navigationService: NavigationService,
+    private scrollService: ScrollService,
     private swUpdateNotifications: SwUpdateNotificationsService
   ) {  }
 
@@ -146,7 +146,7 @@ export class AppComponent implements OnInit {
 
   // Scroll to the anchor in the hash fragment or top of doc.
   autoScroll() {
-    this.autoScrollService.scroll();
+    this.scrollService.scroll();
   }
 
   onDocRendered() {

aio/src/app/app.module.ts

@@ -16,7 +16,6 @@ import { SwUpdatesModule } from 'app/sw-updates/sw-updates.module';
 
 import { AppComponent } from 'app/app.component';
 import { ApiService } from 'app/embedded/api/api.service';
-import { AutoScrollService } from 'app/shared/auto-scroll.service';
 import { CustomMdIconRegistry, SVG_ICONS } from 'app/shared/custom-md-icon-registry';
 import { DocViewerComponent } from 'app/layout/doc-viewer/doc-viewer.component';
 import { DtComponent } from 'app/layout/doc-viewer/dt.component';
@@ -31,6 +30,7 @@ import { TopMenuComponent } from 'app/layout/top-menu/top-menu.component';
 import { FooterComponent } from 'app/layout/footer/footer.component';
 import { NavMenuComponent } from 'app/layout/nav-menu/nav-menu.component';
 import { NavItemComponent } from 'app/layout/nav-item/nav-item.component';
+import { ScrollService } from 'app/shared/scroll.service';
 import { SearchResultsComponent } from './search/search-results/search-results.component';
 import { SearchBoxComponent } from './search/search-box/search-box.component';
 import { TocService } from 'app/shared/toc.service';
@@ -84,7 +84,6 @@ export const svgIconProviders = [
   ],
   providers: [
     ApiService,
-    AutoScrollService,
     DocumentService,
     GaService,
     Logger,
@@ -93,8 +92,9 @@ export const svgIconProviders = [
     LocationService,
     { provide: MdIconRegistry, useClass: CustomMdIconRegistry },
     NavigationService,
-    SearchService,
     Platform,
+    ScrollService,
+    SearchService,
     svgIconProviders,
     TocService
   ],

aio/src/app/embedded/toc/toc.component.spec.ts

@@ -3,6 +3,7 @@ import { async, ComponentFixture, TestBed } from '@angular/core/testing';
 import { By, DOCUMENT } from '@angular/platform-browser';
 import { BehaviorSubject } from 'rxjs/BehaviorSubject';
 
+import { ScrollService } from 'app/shared/scroll.service';
 import { TocComponent } from './toc.component';
 import { TocItem, TocService } from 'app/shared/toc.service';
 
@@ -31,6 +32,7 @@ describe('TocComponent', () => {
     TestBed.configureTestingModule({
       declarations: [ HostEmbeddedTocComponent, HostNotEmbeddedTocComponent, TocComponent ],
       providers: [
+        { provide: ScrollService, useClass: TestScrollService },
         { provide: TocService, useClass: TestTocService }
       ]
     })
@@ -116,7 +118,7 @@ describe('TocComponent', () => {
       beforeEach(() => {
         fixture.detectChanges();
         page = setPage();
-        scrollSpy = spyOn(tocComponent, 'scrollToTop');
+        scrollSpy = TestBed.get(ScrollService).scrollToTop;
       });
 
       it('should have more than 4 displayed items', () => {
@@ -280,6 +282,10 @@ class HostEmbeddedTocComponent {}
 })
 class HostNotEmbeddedTocComponent {}
 
+class TestScrollService {
+  scrollToTop = jasmine.createSpy('scrollToTop');
+}
+
 class TestTocService {
   tocList = new BehaviorSubject<TocItem[]>(getTestTocList());
 }

aio/src/app/embedded/toc/toc.component.ts

@@ -2,6 +2,7 @@ import { Component, ElementRef, OnDestroy, OnInit } from '@angular/core';
 import { Subject } from 'rxjs/Subject';
 import 'rxjs/add/operator/takeUntil';
 
+import { ScrollService } from 'app/shared/scroll.service';
 import { TocItem, TocService } from 'app/shared/toc.service';
 
 @Component({
@@ -21,6 +22,7 @@ export class TocComponent implements OnInit, OnDestroy {
   tocList: TocItem[];
 
   constructor(
+    private scrollService: ScrollService,
     elementRef: ElementRef,
     private tocService: TocService) {
     this.hostElement = elementRef.nativeElement;
@@ -49,13 +51,8 @@ export class TocComponent implements OnInit, OnDestroy {
     this.onDestroy.next();
   }
 
-  scrollToTop() {
-    this.hostElement.parentElement.scrollIntoView();
-    if (window && window.scrollBy) { window.scrollBy(0, -100); }
-  }
-
   toggle() {
     this.isClosed = !this.isClosed;
-    if (this.isClosed) { this.scrollToTop(); }
+    if (this.isClosed) { this.scrollService.scrollToTop(); }
   }
 }

aio/src/app/shared/auto-scroll.service.spec.ts

@@ -1,70 +0,0 @@
-import { ReflectiveInjector } from '@angular/core';
-import { PlatformLocation } from '@angular/common';
-import { DOCUMENT } from '@angular/platform-browser';
-import { AutoScrollService } from './auto-scroll.service';
-
-describe('AutoScrollService', () => {
-  let injector: ReflectiveInjector,
-      autoScroll: AutoScrollService,
-      location: MockPlatformLocation,
-      document: MockDocument;
-
-  class MockPlatformLocation {
-    hash: string;
-  }
-
-  class MockDocument {
-    body = new MockElement();
-    getElementById = jasmine.createSpy('Document getElementById');
-  }
-
-  class MockElement {
-    scrollIntoView = jasmine.createSpy('Element scrollIntoView');
-  }
-
-  beforeEach(() => {
-    spyOn(window, 'scrollBy');
-  });
-
-  beforeEach(() => {
-    injector = ReflectiveInjector.resolveAndCreate([
-        AutoScrollService,
-        { provide: DOCUMENT, useClass: MockDocument },
-        { provide: PlatformLocation, useClass: MockPlatformLocation }
-    ]);
-    location = injector.get(PlatformLocation);
-    document = injector.get(DOCUMENT);
-    autoScroll = injector.get(AutoScrollService);
-  });
-
-  it('should scroll to the top if there is no hash', () => {
-    location.hash = '';
-
-    const topOfPage = new MockElement();
-    document.getElementById.and
-            .callFake(id  => id === 'top-of-page' ? topOfPage : null);
-
-    autoScroll.scroll();
-    expect(topOfPage.scrollIntoView).toHaveBeenCalled();
-  });
-
-  it('should not scroll if the hash does not match an element id', () => {
-    location.hash = 'not-found';
-    document.getElementById.and.returnValue(null);
-
-    autoScroll.scroll();
-    expect(document.getElementById).toHaveBeenCalledWith('not-found');
-    expect(window.scrollBy).not.toHaveBeenCalled();
-  });
-
-  it('should scroll to the element whose id matches the hash', () => {
-    const element = new MockElement();
-    location.hash = 'some-id';
-    document.getElementById.and.returnValue(element);
-
-    autoScroll.scroll();
-    expect(document.getElementById).toHaveBeenCalledWith('some-id');
-    expect(element.scrollIntoView).toHaveBeenCalled();
-    expect(window.scrollBy).toHaveBeenCalledWith(0, -80);
-  });
-});

aio/src/app/shared/auto-scroll.service.ts

@@ -1,37 +0,0 @@
-import { Injectable, Inject, InjectionToken } from '@angular/core';
-import { PlatformLocation } from '@angular/common';
-import { DOCUMENT } from '@angular/platform-browser';
-
-/**
- * A service that supports automatically scrolling elements into view
- */
-@Injectable()
-export class AutoScrollService {
-  constructor(
-      @Inject(DOCUMENT) private document: any,
-      private location: PlatformLocation) { }
-
-  /**
-   * Scroll to the element with id extracted from the current location hash fragment
-   * Scroll to top if no hash
-   * Don't scroll if hash not found
-   */
-  scroll() {
-    const hash = this.getCurrentHash();
-    const element: HTMLElement = hash
-        ? this.document.getElementById(hash)
-        : this.document.getElementById('top-of-page') || this.document.body;
-    if (element) {
-      element.scrollIntoView();
-      if (window && window.scrollBy) { window.scrollBy(0, -80); }
-    }
-  }
-
-  /**
-   * We can get the hash fragment from the `PlatformLocation` but
-   * it needs the `#` char removing from the front.
-   */
-  private getCurrentHash() {
-    return this.location.hash.replace(/^#/, '');
-  }
-}

aio/src/app/shared/scroll.service.spec.ts

@@ -0,0 +1,102 @@
+import { ReflectiveInjector } from '@angular/core';
+import { PlatformLocation } from '@angular/common';
+import { DOCUMENT } from '@angular/platform-browser';
+
+import { ScrollService, topMargin } from './scroll.service';
+
+describe('ScrollService', () => {
+  let injector: ReflectiveInjector;
+  let document: MockDocument;
+  let location: MockPlatformLocation;
+  let scrollService: ScrollService;
+
+  class MockPlatformLocation {
+    hash: string;
+  }
+
+  class MockDocument {
+    body = new MockElement();
+    getElementById = jasmine.createSpy('Document getElementById');
+    querySelector = jasmine.createSpy('Document querySelector');
+  }
+
+  class MockElement {
+    scrollIntoView = jasmine.createSpy('Element scrollIntoView');
+  }
+
+  beforeEach(() => {
+    spyOn(window, 'scrollBy');
+  });
+
+  beforeEach(() => {
+    injector = ReflectiveInjector.resolveAndCreate([
+        ScrollService,
+        { provide: DOCUMENT, useClass: MockDocument },
+        { provide: PlatformLocation, useClass: MockPlatformLocation }
+    ]);
+    location = injector.get(PlatformLocation);
+    document = injector.get(DOCUMENT);
+    scrollService = injector.get(ScrollService);
+  });
+
+  describe('#scroll', () => {
+    it('should scroll to the top if there is no hash', () => {
+      location.hash = '';
+
+      const topOfPage = new MockElement();
+      document.getElementById.and
+              .callFake(id  => id === 'top-of-page' ? topOfPage : null);
+
+      scrollService.scroll();
+      expect(topOfPage.scrollIntoView).toHaveBeenCalled();
+    });
+
+    it('should not scroll if the hash does not match an element id', () => {
+      location.hash = 'not-found';
+      document.getElementById.and.returnValue(null);
+
+      scrollService.scroll();
+      expect(document.getElementById).toHaveBeenCalledWith('not-found');
+      expect(window.scrollBy).not.toHaveBeenCalled();
+    });
+
+    it('should scroll to the element whose id matches the hash', () => {
+      const element = new MockElement();
+      location.hash = 'some-id';
+      document.getElementById.and.returnValue(element);
+
+      scrollService.scroll();
+      expect(document.getElementById).toHaveBeenCalledWith('some-id');
+      expect(element.scrollIntoView).toHaveBeenCalled();
+      expect(window.scrollBy).toHaveBeenCalled();
+    });
+  });
+
+  describe('#scrollToElement', () => {
+    it('should scroll to element', () => {
+      const element = <Element><any> new MockElement();
+      scrollService.scrollToElement(element);
+      expect(element.scrollIntoView).toHaveBeenCalled();
+      expect(window.scrollBy).toHaveBeenCalledWith(0, -topMargin);
+    });
+
+    it('should do nothing if no element', () => {
+      scrollService.scrollToElement(null);
+      expect(window.scrollBy).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('#scrollToTop', () => {
+    it('should scroll to top', () => {
+      const topOfPageElement = <Element><any> new MockElement();
+      document.getElementById.and.callFake(
+        id => id === 'top-of-page' ? topOfPageElement : null
+      );
+
+      scrollService.scrollToTop();
+      expect(topOfPageElement.scrollIntoView).toHaveBeenCalled();
+      expect(window.scrollBy).toHaveBeenCalledWith(0, -topMargin);
+    });
+  });
+
+});

aio/src/app/shared/scroll.service.ts

@@ -0,0 +1,70 @@
+import { Injectable, Inject, InjectionToken } from '@angular/core';
+import { PlatformLocation } from '@angular/common';
+import { DOCUMENT } from '@angular/platform-browser';
+
+export const topMargin = 16;
+/**
+ * A service that scrolls document elements into view
+ */
+@Injectable()
+export class ScrollService {
+
+  private _topOffset: number;
+  private _topOfPageElement: Element;
+
+  constructor(
+      @Inject(DOCUMENT) private document: any,
+      private location: PlatformLocation) { }
+
+  /**
+   * Scroll to the element with id extracted from the current location hash fragment.
+   * Scroll to top if no hash.
+   * Don't scroll if hash not found.
+   */
+  scroll() {
+    const hash = this.getCurrentHash();
+    const element: HTMLElement = hash
+        ? this.document.getElementById(hash)
+        : this.topOfPageElement;
+    this.scrollToElement(element);
+  }
+
+  /**
+   * Scroll to the element.
+   * Don't scroll if no element.
+   */
+  scrollToElement(element: Element) {
+    if (element) {
+      element.scrollIntoView();
+      if (window && window.scrollBy) { window.scrollBy(0, -this.topOffset); }
+    }
+  }
+
+  /** Scroll to the top of the document. */
+  scrollToTop() {
+    this.scrollToElement(this.topOfPageElement);
+  }
+
+  /**
+   * Return the hash fragment from the `PlatformLocation`, minus the leading `#`.
+   */
+  private getCurrentHash() {
+    return this.location.hash.replace(/^#/, '');
+  }
+
+  /** Offset from the top of the document to bottom of toolbar + some margin */
+  private get topOffset() {
+    if (!this._topOffset) {
+      const toolbar = document.querySelector('md-toolbar.app-toolbar');
+      this._topOffset = (toolbar ? toolbar.clientHeight : 0) + topMargin;
+    }
+    return this._topOffset;
+  }
+
+  private get topOfPageElement() {
+    if (!this._topOfPageElement) {
+      this._topOfPageElement = this.document.getElementById('top-of-page') || this.document.body;
+    }
+    return this._topOfPageElement;
+  }
+}