From 26ef99c12149fcdc7fd6f9ac299b6936662d04cc Mon Sep 17 00:00:00 2001 From: Andrew Seguin Date: Tue, 13 Dec 2016 13:21:29 -0800 Subject: [PATCH] review response --- src/lib/core/core.ts | 10 ++++---- ...roll.spec.ts => scroll-dispatcher.spec.ts} | 20 +++++++++------- .../{scroll.ts => scroll-dispatcher.ts} | 19 ++++++++------- src/lib/core/scroll/scrollable.ts | 24 +++++++------------ src/lib/sidenav/sidenav.html | 2 +- src/lib/sidenav/sidenav.ts | 6 ++--- src/lib/tooltip/tooltip.ts | 6 ++--- tools/gulp/tasks/components.ts | 1 + 8 files changed, 43 insertions(+), 45 deletions(-) rename src/lib/core/scroll/{scroll.spec.ts => scroll-dispatcher.spec.ts} (78%) rename src/lib/core/scroll/{scroll.ts => scroll-dispatcher.ts} (79%) diff --git a/src/lib/core/core.ts b/src/lib/core/core.ts index a672690d97e4..68034b044e62 100644 --- a/src/lib/core/core.ts +++ b/src/lib/core/core.ts @@ -6,7 +6,7 @@ import {PortalModule} from './portal/portal-directives'; import {OverlayModule} from './overlay/overlay-directives'; import {A11yModule, A11Y_PROVIDERS} from './a11y/index'; import {OVERLAY_PROVIDERS} from './overlay/overlay'; -import {Scroll} from './scroll/scroll'; +import {ScrollDispatcher} from './scroll/scroll-dispatcher'; import {ScrollModule} from './scroll/scrollable'; @@ -48,7 +48,7 @@ export { export * from './overlay/position/connected-position-strategy'; export * from './overlay/position/connected-position'; export * from './scroll/scrollable'; -export * from './scroll/scroll'; +export * from './scroll/scroll-dispatcher'; // Gestures export {MdGestureConfig} from './gestures/MdGestureConfig'; @@ -101,8 +101,8 @@ export {coerceNumberProperty} from './coersion/number-property'; export {DefaultStyleCompatibilityModeModule} from './compatibility/default-mode'; export {NoConflictStyleCompatibilityMode} from './compatibility/no-conflict-mode'; -// Scroll -export {Scroll} from './scroll/scroll'; +// ScrollDispatcher +export {ScrollDispatcher} from './scroll/scroll-dispatcher'; export {Scrollable} from './scroll/scrollable'; @NgModule({ @@ -127,7 +127,7 @@ export class MdCoreModule { static forRoot(): ModuleWithProviders { return { ngModule: MdCoreModule, - providers: [A11Y_PROVIDERS, OVERLAY_PROVIDERS, Scroll], + providers: [A11Y_PROVIDERS, OVERLAY_PROVIDERS, ScrollDispatcher], }; } } diff --git a/src/lib/core/scroll/scroll.spec.ts b/src/lib/core/scroll/scroll-dispatcher.spec.ts similarity index 78% rename from src/lib/core/scroll/scroll.spec.ts rename to src/lib/core/scroll/scroll-dispatcher.spec.ts index 91821a950a29..2d476e22deaa 100644 --- a/src/lib/core/scroll/scroll.spec.ts +++ b/src/lib/core/scroll/scroll-dispatcher.spec.ts @@ -1,10 +1,10 @@ import {inject, TestBed, async, ComponentFixture} from '@angular/core/testing'; import {NgModule, Component, ViewChild, ElementRef} from '@angular/core'; -import {Scroll} from './scroll'; +import {ScrollDispatcher} from './scroll-dispatcher'; import {ScrollModule, Scrollable} from './scrollable'; -describe('Scrollable', () => { - let scroll: Scroll; +describe('Scroll Dispatcher', () => { + let scroll: ScrollDispatcher; let fixture: ComponentFixture; beforeEach(async(() => { @@ -15,19 +15,19 @@ describe('Scrollable', () => { TestBed.compileComponents(); })); - beforeEach(inject([Scroll], (s: Scroll) => { + beforeEach(inject([ScrollDispatcher], (s: ScrollDispatcher) => { scroll = s; fixture = TestBed.createComponent(ScrollingComponent); fixture.detectChanges(); })); - it('should register the scrollable directive with the scroll service', () => { + it('should be registered with the scrollable directive with the scroll service', () => { const componentScrollable = fixture.componentInstance.scrollable; expect(scroll.scrollableReferences.has(componentScrollable)).toBe(true); }); - it('should deregister the scrollable directive when the component is destroyed', () => { + it('should have the scrollable directive deregistered when the component is destroyed', () => { const componentScrollable = fixture.componentInstance.scrollable; expect(scroll.scrollableReferences.has(componentScrollable)).toBe(true); @@ -48,7 +48,9 @@ describe('Scrollable', () => { // Emit a scroll event from the scrolling element in our component. // This event should be picked up by the scrollable directive and notify. // The notification should be picked up by the service. - fixture.componentInstance.scrollingElement.nativeElement.dispatchEvent(new Event('scroll')); + const scrollEvent = document.createEvent('UIEvents'); + scrollEvent.initUIEvent('scroll', true, true, window, 0); + fixture.componentInstance.scrollingElement.nativeElement.dispatchEvent(scrollEvent); expect(hasDirectiveScrollNotified).toBe(true); expect(hasServiceScrollNotified).toBe(true); @@ -58,7 +60,7 @@ describe('Scrollable', () => { /** Simple component that contains a large div and can be scrolled. */ @Component({ - template: `
` + template: `
` }) class ScrollingComponent { @ViewChild(Scrollable) scrollable: Scrollable; @@ -68,7 +70,7 @@ class ScrollingComponent { const TEST_COMPONENTS = [ScrollingComponent]; @NgModule({ imports: [ScrollModule], - providers: [Scroll], + providers: [ScrollDispatcher], exports: TEST_COMPONENTS, declarations: TEST_COMPONENTS, entryComponents: TEST_COMPONENTS, diff --git a/src/lib/core/scroll/scroll.ts b/src/lib/core/scroll/scroll-dispatcher.ts similarity index 79% rename from src/lib/core/scroll/scroll.ts rename to src/lib/core/scroll/scroll-dispatcher.ts index 2e9c823a66d5..ebf49fb7c5cf 100644 --- a/src/lib/core/scroll/scroll.ts +++ b/src/lib/core/scroll/scroll-dispatcher.ts @@ -3,6 +3,7 @@ import {Scrollable} from './scrollable'; import {Subject} from 'rxjs/Subject'; import {Observable} from 'rxjs/Observable'; import {Subscription} from 'rxjs/Subscription'; +import 'rxjs/add/observable/fromEvent'; /** @@ -10,20 +11,20 @@ import {Subscription} from 'rxjs/Subscription'; * Scrollable references emit a scrolled event. */ @Injectable() -export class Scroll { +export class ScrollDispatcher { /** Subject for notifying that a registered scrollable reference element has been scrolled. */ - _scrolled: Subject = new Subject(); + _scrolled: Subject = new Subject(); /** * Map of all the scrollable references that are registered with the service and their * scroll event subscriptions. */ - scrollableReferences: Map = new Map(); + scrollableReferences: WeakMap = new WeakMap(); constructor() { // By default, notify a scroll event when the document is scrolled or the window is resized. - window.document.addEventListener('scroll', this._notify.bind(this)); - window.addEventListener('resize', this._notify.bind(this)); + Observable.fromEvent(window.document, 'scroll').subscribe(() => this._notify()); + Observable.fromEvent(window, 'resize').subscribe(() => this._notify()); } /** @@ -31,7 +32,7 @@ export class Scroll { * scrollable is scrolled, the service emits the event in its scrolled observable. */ register(scrollable: Scrollable): void { - const scrollSubscription = scrollable.elementScrolled().subscribe(this._notify.bind(this)); + const scrollSubscription = scrollable.elementScrolled().subscribe(() => this._notify()); this.scrollableReferences.set(scrollable, scrollSubscription); } @@ -48,12 +49,12 @@ export class Scroll { * references (or window, document, or body) fire a scrolled event. * TODO: Add an event limiter that includes throttle with the leading and trailing events. */ - scrolled(): Observable { + scrolled(): Observable { return this._scrolled.asObservable(); } /** Sends a notification that a scroll event has been fired. */ - _notify(e: Event) { - this._scrolled.next(e); + _notify() { + this._scrolled.next(); } } diff --git a/src/lib/core/scroll/scrollable.ts b/src/lib/core/scroll/scrollable.ts index 5998a8a17874..eb0c78b13f4d 100644 --- a/src/lib/core/scroll/scrollable.ts +++ b/src/lib/core/scroll/scrollable.ts @@ -2,39 +2,33 @@ import { Directive, ElementRef, OnInit, OnDestroy, ModuleWithProviders, NgModule } from '@angular/core'; -import {Subject} from 'rxjs/Subject'; import {Observable} from 'rxjs/Observable'; -import {Scroll} from './scroll'; +import {ScrollDispatcher} from './scroll-dispatcher'; +import 'rxjs/add/observable/fromEvent'; /** - * Sends an event when the directive's element is scrolled. Registers itself with the Scroll + * Sends an event when the directive's element is scrolled. Registers itself with the ScrollDispatcher * service to include itself as part of its collection of scrolling events that it can be listened * to through the service. */ @Directive({ - selector: '[md-scrollable]' + selector: '[cdk-scrollable]' }) export class Scrollable implements OnInit, OnDestroy { - /** Subject for notifying that the element has been scrolled. */ - private _elementScrolled: Subject = new Subject(); - - constructor(private _elementRef: ElementRef, private _scroll: Scroll) {} + constructor(private _elementRef: ElementRef, private _scroll: ScrollDispatcher) {} ngOnInit() { this._scroll.register(this); - this._elementRef.nativeElement.addEventListener('scroll', (e: Event) => { - this._elementScrolled.next(e); - }); } ngOnDestroy() { this._scroll.deregister(this); } - /** Returns observable that emits an event when the scroll event is fired on the host element. */ - elementScrolled(): Observable { - return this._elementScrolled.asObservable(); + /** Returns observable that emits when the scroll event is fired on the host element. */ + elementScrolled(): Observable { + return Observable.fromEvent(this._elementRef.nativeElement, 'scroll'); } } @@ -47,7 +41,7 @@ export class ScrollModule { static forRoot(): ModuleWithProviders { return { ngModule: ScrollModule, - providers: [Scroll] + providers: [ScrollDispatcher] }; } } diff --git a/src/lib/sidenav/sidenav.html b/src/lib/sidenav/sidenav.html index dae02bd1b0e5..c916ef399af4 100644 --- a/src/lib/sidenav/sidenav.html +++ b/src/lib/sidenav/sidenav.html @@ -3,6 +3,6 @@ -
+
diff --git a/src/lib/sidenav/sidenav.ts b/src/lib/sidenav/sidenav.ts index 2d59377a3458..b69107b980b3 100644 --- a/src/lib/sidenav/sidenav.ts +++ b/src/lib/sidenav/sidenav.ts @@ -17,14 +17,14 @@ import { } from '@angular/core'; import {CommonModule} from '@angular/common'; import {Dir, MdError, coerceBooleanProperty, DefaultStyleCompatibilityModeModule} from '../core'; -import {A11yModule, A11Y_PROVIDERS} from '../core/a11y/index'; +import {A11yModule} from '../core/a11y/index'; import {FocusTrap} from '../core/a11y/focus-trap'; import {ESCAPE} from '../core/keyboard/keycodes'; import {OverlayModule} from '../core/overlay/overlay-directives'; import {ScrollModule} from '../core/scroll/scrollable'; import {InteractivityChecker} from '../core/a11y/interactivity-checker'; import {MdLiveAnnouncer} from '../core/a11y/live-announcer'; -import {Scroll} from '../core/scroll/scroll'; +import {ScrollDispatcher} from '../core/scroll/scroll-dispatcher'; /** Exception thrown when two MdSidenav are matching the same side. */ @@ -522,7 +522,7 @@ export class MdSidenavModule { static forRoot(): ModuleWithProviders { return { ngModule: MdSidenavModule, - providers: [MdLiveAnnouncer, InteractivityChecker, Scroll] + providers: [MdLiveAnnouncer, InteractivityChecker, ScrollDispatcher] }; } } diff --git a/src/lib/tooltip/tooltip.ts b/src/lib/tooltip/tooltip.ts index 20e6e20e8526..776b92ccd880 100644 --- a/src/lib/tooltip/tooltip.ts +++ b/src/lib/tooltip/tooltip.ts @@ -29,7 +29,7 @@ import {MdTooltipInvalidPositionError} from './tooltip-errors'; import {Observable} from 'rxjs/Observable'; import {Subject} from 'rxjs/Subject'; import {Dir} from '../core/rtl/dir'; -import {Scroll} from '../core/scroll/scroll'; +import {ScrollDispatcher} from '../core/scroll/scroll-dispatcher'; import {ScrollModule} from '../core/scroll/scrollable'; import {OverlayPositionBuilder} from '../core/overlay/position/overlay-position-builder'; import {ViewportRuler} from '../core/overlay/position/viewport-ruler'; @@ -96,7 +96,7 @@ export class MdTooltip implements OnInit, OnDestroy { } constructor(private _overlay: Overlay, - private _scroll: Scroll, + private _scroll: ScrollDispatcher, private _elementRef: ElementRef, private _viewContainerRef: ViewContainerRef, private _ngZone: NgZone, @@ -378,7 +378,7 @@ export class MdTooltipModule { Overlay, OverlayPositionBuilder, ViewportRuler, - Scroll + ScrollDispatcher ] }; } diff --git a/tools/gulp/tasks/components.ts b/tools/gulp/tasks/components.ts index fcf54b593598..7d02d692fbed 100644 --- a/tools/gulp/tasks/components.ts +++ b/tools/gulp/tasks/components.ts @@ -67,6 +67,7 @@ task(':build:components:rollup', [':build:components:inline'], () => { // Rxjs dependencies 'rxjs/Subject': 'Rx', + 'rxjs/add/observable/fromEvent': 'Rx.Observable', 'rxjs/add/observable/forkJoin': 'Rx.Observable', 'rxjs/add/observable/of': 'Rx.Observable', 'rxjs/add/operator/toPromise': 'Rx.Observable.prototype',