feat(scroll): provide directive and service to listen to scrolling #2188

andrewseguin · 2016-12-13T00:26:49Z

Includes applying the directive to the sidenav as an example of usage. The tooltip makes use of the scroll service's notification and updates in position.

Would be nice to have some optimization on throttling the scroll events, ideally something similar to throttle but includes the trailing event.

jelbourn · 2016-12-13T01:14:12Z

src/lib/core/scroll/scroll.ts

+ * Scrollable references emit a scrolled event.
+ */
+@Injectable()
+export class Scroll {


I would call this ScrollMonitor or ScrollDispatcher (I'm partial to the latter)

jelbourn · 2016-12-13T01:14:29Z

src/lib/core/scroll/scroll.ts

+   * Map of all the scrollable references that are registered with the service and their
+   * scroll event subscriptions.
+   */
+  scrollableReferences: Map<Scrollable, Subscription> = new Map();


Use a WeakMap

jelbourn · 2016-12-13T01:15:32Z

src/lib/core/scroll/scroll.ts

+  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));


Prefer arrow functions to .bind:

window.document.addEventListener('scroll', () => this.notify());

(here and elsewhere)

this.renderer.listenGlobal('window', 'resize', (e: Event) => {})

jelbourn · 2016-12-13T01:16:49Z

src/lib/core/scroll/scroll.ts

+@Injectable()
+export class Scroll {
+  /** Subject for notifying that a registered scrollable reference element has been scrolled. */
+  _scrolled: Subject<Event> = new Subject();


Do you need the Event, or can this be void?

Right now I don't have a need for it. I can remove the event until it is required.

jelbourn · 2016-12-13T01:19:20Z

src/lib/core/scroll/scrollable.ts

+ * to through the service.
+ */
+@Directive({
+  selector: '[md-scrollable]'


Let's make this cdk-scrollable (it's starting!)

jelbourn · 2016-12-13T01:19:54Z

src/lib/core/scroll/scrollable.ts

+  exports: [Scrollable],
+  declarations: [Scrollable],
+})
+export class ScrollModule {


I'm not sure this needs its own module vs. just being a part of OverlayModule. Thoughts?

jelbourn · 2016-12-13T01:22:14Z

src/lib/core/scroll/scroll.spec.ts

+    // 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'));


new Event isn't supported in IE11

crisbeto · 2016-12-13T08:14:30Z

src/lib/core/scroll/scrollable.ts

+
+  ngOnInit() {
+    this._scroll.register(this);
+    this._elementRef.nativeElement.addEventListener('scroll', (e: Event) => {


Instead of binding the event directly, I think you could use Observable.fromEvent. Also I think the same can be done above in the service.

DzmitryShylovich · 2016-12-13T08:15:27Z

src/lib/core/scroll/scrollable.ts

+
+  ngOnInit() {
+    this._scroll.register(this);
+    this._elementRef.nativeElement.addEventListener('scroll', (e: Event) => {


this.renderer.listen(this._elementRef.nativeElement, 'scroll', (e: Event) => {})

DzmitryShylovich · 2016-12-13T08:17:40Z

src/lib/core/scroll/scroll.ts

+
+  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));


this.renderer.listenGlobal('document', 'scroll', (e: Event) => {})

to make it universal friendly

or it would be even better to use @Component.host property to listen for events

crisbeto · 2016-12-13T08:17:45Z

src/lib/core/scroll/scroll.ts

+
+  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));


It might be better to bind this only after a component has been registered. Also we could unbind it when there are no subscriptions.

I considered this but then we miss out in the case of the document body scrolling without any scrollables on the page. E.g. a long document body with a tooltip.

In the current implementation, the listeners are only attached if at least one component uses another component (e.g. tooltip) that injects the ScrollDispatcher service.

Sounds good. Also something else that I was thinking about: I'm pretty sure that this event will trigger Angular's change detection. It may be worth trying to run it outside Angular's zone, however I'm not sure whether it won't end up not updating the view at the proper times.

DzmitryShylovich · 2016-12-13T22:00:42Z

src/lib/core/scroll/scroll-dispatcher.ts

+  constructor() {
+    // By default, notify a scroll event when the document is scrolled or the window is resized.
+    Observable.fromEvent(window.document, 'scroll').subscribe(() => this._notify());
+    Observable.fromEvent(window, 'resize').subscribe(() => this._notify());


still won't work in universal.
Plus I believe u need to unsubscribe from it on destroy

And I seriously don't see why u want to imperatively subscribe here if u can just use @Component.host. it works in universal and automatically unsubscribes on destroy.

Right now we are not prioritizing universal right now so this may be a concern for another time. Additionally I think there may be some confusion here as this is a service and not a component so it does not have access to a host nor have a lifecycle with destroy.

nor have a lifecycle with destroy.

it's not true. services do have OnDestroy hook :)

is a service and not a component so it does not have access to a host

right, I missed that part. ignore my comments

andrewseguin · 2016-12-13T22:30:41Z

Moved the scroll dispatcher and scrollable directive to the overlay folder.

jelbourn

LGTM, one nit

jelbourn · 2016-12-14T03:06:04Z

src/lib/tooltip/tooltip.ts

-  constructor(private _overlay: Overlay, private _elementRef: ElementRef,
-              private _viewContainerRef: ViewContainerRef, private _ngZone: NgZone,
+  constructor(private _overlay: Overlay,
+              private _scroll: ScrollDispatcher,


_scrollDispatcher

andrewseguin · 2016-12-14T19:57:03Z

Changed _scroll to _scrollDispatcher

andrewseguin · 2016-12-19T20:54:40Z

Rebased

angular-automatic-lock-bot · 2019-09-06T05:03:02Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

andrewseguin added the pr: needs review label Dec 13, 2016

andrewseguin requested a review from jelbourn December 13, 2016 00:26

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 13, 2016

jelbourn reviewed Dec 13, 2016

View reviewed changes

crisbeto reviewed Dec 13, 2016

View reviewed changes

DzmitryShylovich reviewed Dec 13, 2016

View reviewed changes

crisbeto reviewed Dec 13, 2016

View reviewed changes

andrewseguin force-pushed the overlay-scroll branch from 378281d to 26ef99c Compare December 13, 2016 21:21

DzmitryShylovich reviewed Dec 13, 2016

View reviewed changes

andrewseguin force-pushed the overlay-scroll branch from d1029e4 to f426670 Compare December 13, 2016 22:26

jelbourn reviewed Dec 14, 2016

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Dec 14, 2016

andrewseguin force-pushed the overlay-scroll branch from f426670 to 8aefb13 Compare December 14, 2016 19:55

andrewseguin mentioned this pull request Dec 16, 2016

feat(core): add scrollable view properties to connected pos strategy #2259

Merged

jelbourn added the action: merge The PR is ready for merge by the caretaker label Dec 16, 2016

andrewseguin force-pushed the overlay-scroll branch from 8aefb13 to e22b009 Compare December 19, 2016 20:54

andrewseguin force-pushed the overlay-scroll branch from e22b009 to 95652a6 Compare December 20, 2016 18:30

andrewseguin added 5 commits December 20, 2016 11:03

feat(scroll): provide directive and service to listen to scrolling

857b7d1

review response

c959640

fix lint

f4b5aad

move scroll to overlay

c5223e2

rename scroll in tooltip

9159368

andrewseguin force-pushed the overlay-scroll branch from f1745d1 to 9159368 Compare December 20, 2016 19:04

remove reference tot he live announcer

c23475f

jelbourn merged commit 9b68e68 into angular:master Dec 20, 2016

andrewseguin deleted the overlay-scroll branch December 20, 2016 22:57

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scroll): provide directive and service to listen to scrolling #2188

feat(scroll): provide directive and service to listen to scrolling #2188

andrewseguin commented Dec 13, 2016 •

edited

Loading

jelbourn Dec 13, 2016

jelbourn Dec 13, 2016

jelbourn Dec 13, 2016

DzmitryShylovich Dec 13, 2016

jelbourn Dec 13, 2016

andrewseguin Dec 13, 2016

jelbourn Dec 13, 2016

jelbourn Dec 13, 2016

jelbourn Dec 13, 2016

crisbeto Dec 13, 2016

DzmitryShylovich Dec 13, 2016

DzmitryShylovich Dec 13, 2016 •

edited

Loading

crisbeto Dec 13, 2016

andrewseguin Dec 13, 2016 •

edited

Loading

crisbeto Dec 13, 2016 •

edited

Loading

DzmitryShylovich Dec 13, 2016 •

edited

Loading

andrewseguin Dec 13, 2016

DzmitryShylovich Dec 13, 2016 •

edited

Loading

andrewseguin commented Dec 13, 2016

jelbourn left a comment

jelbourn Dec 14, 2016

andrewseguin commented Dec 14, 2016

andrewseguin commented Dec 19, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

feat(scroll): provide directive and service to listen to scrolling #2188

feat(scroll): provide directive and service to listen to scrolling #2188

Conversation

andrewseguin commented Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DzmitryShylovich Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

crisbeto Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

DzmitryShylovich Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DzmitryShylovich Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

andrewseguin commented Dec 13, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Dec 14, 2016

andrewseguin commented Dec 19, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

andrewseguin commented Dec 13, 2016 •

edited

Loading

DzmitryShylovich Dec 13, 2016 •

edited

Loading

andrewseguin Dec 13, 2016 •

edited

Loading

crisbeto Dec 13, 2016 •

edited

Loading

DzmitryShylovich Dec 13, 2016 •

edited

Loading

DzmitryShylovich Dec 13, 2016 •

edited

Loading