Skip to content

Commit

Permalink
review response
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewseguin committed Dec 13, 2016
1 parent dffac63 commit 26ef99c
Show file tree
Hide file tree
Showing 8 changed files with 43 additions and 45 deletions.
10 changes: 5 additions & 5 deletions src/lib/core/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';


Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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({
Expand All @@ -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],
};
}
}
Original file line number Diff line number Diff line change
@@ -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<ScrollingComponent>;

beforeEach(async(() => {
Expand All @@ -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);

Expand All @@ -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);
Expand All @@ -58,7 +60,7 @@ describe('Scrollable', () => {

/** Simple component that contains a large div and can be scrolled. */
@Component({
template: `<div #scrollingElement md-scrollable style="height: 9999px"></div>`
template: `<div #scrollingElement cdk-scrollable style="height: 9999px"></div>`
})
class ScrollingComponent {
@ViewChild(Scrollable) scrollable: Scrollable;
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,35 +3,36 @@ import {Scrollable} from './scrollable';
import {Subject} from 'rxjs/Subject';
import {Observable} from 'rxjs/Observable';
import {Subscription} from 'rxjs/Subscription';
import 'rxjs/add/observable/fromEvent';


/**
* Service contained all registered Scrollable references and emits an event when any one of the
* 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<Event> = new Subject();
_scrolled: Subject<void> = new Subject<void>();

/**
* Map of all the scrollable references that are registered with the service and their
* scroll event subscriptions.
*/
scrollableReferences: Map<Scrollable, Subscription> = new Map();
scrollableReferences: WeakMap<Scrollable, Subscription> = 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());
}

/**
* Registers a Scrollable with the service and listens for its scrolled events. When the
* 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);
}

Expand All @@ -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<Event> {
scrolled(): Observable<void> {
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();
}
}
24 changes: 9 additions & 15 deletions src/lib/core/scroll/scrollable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Event> = 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<Event> {
return this._elementScrolled.asObservable();
/** Returns observable that emits when the scroll event is fired on the host element. */
elementScrolled(): Observable<any> {
return Observable.fromEvent(this._elementRef.nativeElement, 'scroll');
}
}

Expand All @@ -47,7 +41,7 @@ export class ScrollModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: ScrollModule,
providers: [Scroll]
providers: [ScrollDispatcher]
};
}
}
2 changes: 1 addition & 1 deletion src/lib/sidenav/sidenav.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@

<ng-content select="md-sidenav, mat-sidenav"></ng-content>

<div class="md-sidenav-content" [ngStyle]="_getStyles()" md-scrollable>
<div class="md-sidenav-content" [ngStyle]="_getStyles()" cdk-scrollable>
<ng-content></ng-content>
</div>
6 changes: 3 additions & 3 deletions src/lib/sidenav/sidenav.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -522,7 +522,7 @@ export class MdSidenavModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: MdSidenavModule,
providers: [MdLiveAnnouncer, InteractivityChecker, Scroll]
providers: [MdLiveAnnouncer, InteractivityChecker, ScrollDispatcher]
};
}
}
6 changes: 3 additions & 3 deletions src/lib/tooltip/tooltip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -378,7 +378,7 @@ export class MdTooltipModule {
Overlay,
OverlayPositionBuilder,
ViewportRuler,
Scroll
ScrollDispatcher
]
};
}
Expand Down
1 change: 1 addition & 0 deletions tools/gulp/tasks/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down

0 comments on commit 26ef99c

Please sign in to comment.