fix(tabs): observing tab header label changes to recalculate width #2186

EladBezalel · 2016-12-12T20:53:46Z

Uses observe-changes directive that emit an event when the mutation observer notifies a change

crisbeto · 2016-12-12T21:42:27Z

src/lib/core/observe-content/observe-content.ts

+  constructor(private elementRef: ElementRef) {}
+
+  ngAfterViewInit() {
+    this.observer = new MutationObserver(mutations => mutations.forEach(() => this.event.emit()));


Might be a good idea to clear out the mutation observer on destroy.

jelbourn · 2016-12-12T23:04:29Z

src/lib/core/observe-content/observe-content.ts

+  selector: '[cdk-observe-content]'
+})
+export class MdMutationObserver {
+  private observer: MutationObserver;


We prefix private members with underscore (_observer)

jelbourn · 2016-12-12T23:04:55Z

src/lib/core/observe-content/observe-content.ts

+export class MdMutationObserver {
+  private observer: MutationObserver;
+
+  @Output('cdk-observe-content') event = new EventEmitter();


I think it should be EventEmitter<void>

jelbourn · 2016-12-12T23:05:15Z

src/lib/core/observe-content/observe-content.ts

+
+  @Output('cdk-observe-content') event = new EventEmitter();
+
+  constructor(private elementRef: ElementRef) {}


_elementRef

jelbourn · 2016-12-12T23:05:56Z

src/lib/core/observe-content/observe-content.ts

+
+  constructor(private elementRef: ElementRef) {}
+
+  ngAfterViewInit() {


Should be ngAfterContentInit, since technically a @Directive doesn't have a view, only content.

jelbourn · 2016-12-12T23:09:30Z

src/lib/core/observe-content/observe-content.ts

+  ngAfterViewInit() {
+    this.observer = new MutationObserver(mutations => mutations.forEach(() => this.event.emit()));
+
+    this.observer.observe(this.elementRef.nativeElement, {characterData: true, subtree: true});


Don't need childList?

jelbourn · 2016-12-12T23:10:12Z

src/lib/core/observe-content/observe-content.ts

+@Directive({
+  selector: '[cdk-observe-content]'
+})
+export class MdMutationObserver {


Call this class just ObserveContent

jelbourn · 2016-12-12T23:10:55Z

src/lib/tabs/tab-header.ts

@@ -150,6 +148,12 @@ export class MdTabHeader {
    }
  }

+  _checkPagination() {


_updatePagination? Also needs a method description

jelbourn · 2016-12-12T23:17:04Z

src/lib/core/observe-content/observe-content.spec.ts

+
+      fixture.componentInstance.text = 'text';
+      fixture.detectChanges();
+    });


Why not have below

class ComponentWithTextContent { contentUpdated = jasmine.createSpy('contentUpdated'); }

And then in the test

let fixture = TestBed.createComponent(ComponentWithTextContent); fixture.detectChanges(); fixture.componentInstance.contentUpdated.reset(); fixture.componentInstance.text = 'Taco'; fixture.detectChanges(); expect(fixture.componentInstance.contentUpdated.calls.count()) .toBe(1, 'Expected exactly one content change event'); fixture.componentInstance.text = 'Chocolate'; fixture.detectChanges(); expect(fixture.componentInstance.contentUpdated.calls.count()) .toBe(2, 'Expected exactly two content change events');

(similar for other test)

Just tried this locally and I saw that the async stuff doesn't really work. Seems like zones doesn't play well with MutationObserver...

Hmm, Zones is supposed to understand MutationObserver, I'll have to check with Misko on this tomorrow.

EladBezalel · 2016-12-14T18:51:16Z

@jelbourn @crisbeto please re-review :)

crisbeto · 2016-12-14T18:56:40Z

src/lib/core/observe-content/observe-content.ts

+@Directive({
+  selector: '[cdk-observe-content]'
+})
+export class ObserveContent {


You should add an annotation to the class: export class ObserveContent implements OnDestroy, AfterContentInit.

When you're right, you're right!

jelbourn

Can you fix the lint error?

jelbourn · 2016-12-16T23:05:11Z

src/lib/tabs/tab-header.ts

@@ -150,6 +148,16 @@ export class MdTabHeader {
    }
  }

+  /**
+   * Updating the view whether pagination should be enabled or not
+   * @private


Don't use @private JsDoc, it messes up stuff when we sync into google

jelbourn · 2016-12-20T20:23:44Z

@andrewseguin can you review as well?

andrewseguin

LGTM, small thing about the output

andrewseguin · 2016-12-20T20:56:03Z

src/lib/core/observe-content/observe-content.ts

+export class ObserveContent implements AfterContentInit, OnDestroy {
+  private _observer: MutationObserver;
+
+  @Output('cdk-observe-content') event = new EventEmitter<void>();


Should this be camelcased?

- Uses observe-changes directive that emit an event when the mutation observer notifies a change fixes angular#2155

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

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.}

EladBezalel requested a review from jelbourn December 12, 2016 20:53

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

EladBezalel force-pushed the feat/observe-changes branch from e56f38f to 6ed8bec Compare December 12, 2016 21:12

crisbeto reviewed Dec 12, 2016

View reviewed changes

jelbourn reviewed Dec 13, 2016

View reviewed changes

EladBezalel force-pushed the feat/observe-changes branch 3 times, most recently from 650d09b to 9b2404b Compare December 14, 2016 18:50

crisbeto reviewed Dec 14, 2016

View reviewed changes

EladBezalel force-pushed the feat/observe-changes branch 2 times, most recently from 5fa3c58 to f839831 Compare December 16, 2016 14:48

jelbourn reviewed Dec 16, 2016

View reviewed changes

EladBezalel force-pushed the feat/observe-changes branch 2 times, most recently from 56d06b1 to d0b4fc8 Compare December 19, 2016 22:22

andrewseguin approved these changes Dec 20, 2016

View reviewed changes

fix(tabs): observing tab header label changes to recalculate width

c8a6e6c

- Uses observe-changes directive that emit an event when the mutation observer notifies a change fixes angular#2155

EladBezalel force-pushed the feat/observe-changes branch from d0b4fc8 to c8a6e6c Compare December 20, 2016 21:57

jelbourn merged commit 7ab4430 into angular:master Dec 20, 2016

EladBezalel deleted the feat/observe-changes branch December 21, 2016 07:35

EladBezalel mentioned this pull request Jan 7, 2017

[Tabs] Use MutationObserver to update tab header pagination #2115

Closed

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

fix(tabs): observing tab header label changes to recalculate width #2186

fix(tabs): observing tab header label changes to recalculate width #2186

EladBezalel commented Dec 12, 2016

crisbeto Dec 12, 2016 •

edited

Loading

jelbourn Dec 12, 2016

jelbourn Dec 12, 2016

jelbourn Dec 12, 2016

jelbourn Dec 12, 2016

jelbourn Dec 12, 2016

jelbourn Dec 12, 2016

EladBezalel Dec 13, 2016

jelbourn Dec 12, 2016

jelbourn Dec 12, 2016

jelbourn Dec 13, 2016

jelbourn Dec 13, 2016

EladBezalel commented Dec 14, 2016

crisbeto Dec 14, 2016

EladBezalel Dec 14, 2016

jelbourn left a comment

jelbourn Dec 16, 2016

jelbourn commented Dec 20, 2016

andrewseguin left a comment

andrewseguin Dec 20, 2016

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


		@Output('cdk-observe-content') event = new EventEmitter();

		constructor(private elementRef: ElementRef) {}


		constructor(private elementRef: ElementRef) {}

		ngAfterViewInit() {

fix(tabs): observing tab header label changes to recalculate width #2186

fix(tabs): observing tab header label changes to recalculate width #2186

Conversation

EladBezalel commented Dec 12, 2016

crisbeto Dec 12, 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

Choose a reason for hiding this comment

EladBezalel commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Dec 20, 2016

andrewseguin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

crisbeto Dec 12, 2016 •

edited

Loading