Force change-detection upon ContentChildren()... #8398

actra-gschuster · 2017-11-12T22:42:42Z

...changes like added / remove steps

mmalerba · 2017-11-14T06:03:22Z

src/lib/stepper/stepper.ts

+    this._stepChangesSubscription = this._steps.changes.subscribe(() => this._stateChanged());
+  }
+  ngOnDestroy() {
+    if(this._stepChangesSubscription instanceof Subscription) {


A pattern we've started using for situations like this is to create a stream that emits on destory and then use takeUntil when describing (e.g. https://github.com/angular/material2/blob/master/src/lib/sidenav/drawer.ts#L467)

So MatStep should emit on destroy and MatStepper should subscribe on each step?
To know which steps exist stepper would have to listen on QueryList changes...
Did I get that correctly?

Like this, your ngOnDestroy would emit on a subject:

ngOnDestroy() { this._destroyed.next(); this._destroyed.complete(); }

And then you don't have to save the Subscription when you subscribe, you can just use takeUntil:

this._steps.changes.pipe(takeUntil(this._destroyed)).subscribe(() => ...);

This way if we make more subscriptions later on we don't have to save them all as separate member variables

mmalerba · 2017-11-14T06:03:37Z

src/lib/stepper/stepper.ts

+  _stepChangesSubscription: Subscription;
+  ngAfterContentInit() {
+    this._stepChangesSubscription = this._steps.changes.subscribe(() => this._stateChanged());
+  }


actra-gschuster · 2017-11-15T12:31:00Z

@mmalerba Applied your suggested changes, added the "emit on destroy" pattern to the upper CDK stepper for other inherited classes to be able to use it without specifying their own _destroy property.

willshowell · 2017-11-15T14:26:08Z

src/lib/stepper/stepper.ts

+  /** Workaround for https://github.com/angular/material2/issues/8397 */
+  ngAfterContentInit() {
+    this._steps.changes.pipe(takeUntil(this._destroyed)).subscribe(() => this._stateChanged());
+  }


Maybe change the comment to be like this?

ngAfterContentInit() { // Mark the component for change detection whenever the content children query changes this._steps.changes.pipe(...).subscribe(...); }

Agreed, fixed.

willshowell · 2017-11-15T14:27:30Z

src/cdk/stepper/stepper.ts

 } from '@angular/core';
 import {LEFT_ARROW, RIGHT_ARROW, ENTER, SPACE} from '@angular/cdk/keycodes';
 import {CdkStepLabel} from './step-label';
 import {coerceBooleanProperty} from '@angular/cdk/coercion';
 import {AbstractControl} from '@angular/forms';
 import {Direction, Directionality} from '@angular/cdk/bidi';
+import {takeUntil} from 'rxjs/operators/takeUntil';


I think this import should be in src/lib/stepper/stepper.ts since it isn't used in this file.

Agreed, was a mistake.

mmalerba

looks good, one more nit

mmalerba · 2017-11-15T16:59:18Z

src/lib/stepper/stepper.ts

  /** The list of step headers of the steps in the stepper. */
  @ViewChildren(MatStepHeader, {read: ElementRef}) _stepHeader: QueryList<ElementRef>;

  /** Steps that the stepper holds. */
  @ContentChildren(MatStep) _steps: QueryList<MatStep>;
+
+  /** Mark the component for change detection whenever the content children query changes */


minor nit: move this comment inside the method and use // style comment since it describes the reason behind some specific logic, not the purpose of the ngAfterContentInit method

actra-gschuster · 2017-11-15T21:35:52Z

OK, I think we're done now ;-)

mmalerba · 2017-11-15T22:21:45Z

oh it looks like there's some lint errors and you forgot to import Subject, but good to go after that's fixed

mmalerba · 2017-11-16T23:38:46Z

I think you added the import to the wrong file, CI is still red

actra-gschuster · 2017-11-17T13:34:19Z

Woohoo, tests finally passed, linting is very nitpicking about trailing spaces ;-)

willshowell · 2017-11-17T13:42:59Z

@actra-gschuster for next time, you should be able to set your editor to automatically remove trailing spaces. You can also run tests and lint locally so you don't have to push and wait for travis each time:

npm run test
npm run tslint

...changes like added / remove steps

Moved step changes subscription to AfterContentInit as it depends on `@ContentChildren`

Now uses "emit on destroy" pattern for stepper

Now uses "emit on destroy" pattern for stepper to subscribe to step changes (fixes angular#8397)

Removed (now) unneccessary Subscription import

Fixed missing import

angular-automatic-lock-bot · 2019-09-07T17:03:01Z

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

actra-gschuster requested a review from mmalerba as a code owner November 12, 2017 22:42

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

actra-gschuster mentioned this pull request Nov 12, 2017

Stepper: Removing step does not reflect to header until clicked, then errors due to index = -1 #8397

Closed

jelbourn assigned mmalerba Nov 14, 2017

mmalerba reviewed Nov 14, 2017

View reviewed changes

willshowell reviewed Nov 15, 2017

View reviewed changes

mmalerba reviewed Nov 15, 2017

View reviewed changes

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Nov 15, 2017

mmalerba approved these changes Nov 15, 2017

View reviewed changes

mmalerba removed the action: merge The PR is ready for merge by the caretaker label Nov 15, 2017

mmalerba added the action: merge The PR is ready for merge by the caretaker label Nov 17, 2017

actra-gschuster added 12 commits November 18, 2017 10:36

Force change-detection upon ContentChildren()...

738314d

...changes like added / remove steps

Update stepper.ts

f185f97

Moved step changes subscription to AfterContentInit as it depends on `@ContentChildren`

Update stepper.ts

94384d5

Now uses "emit on destroy" pattern for stepper

Update stepper.ts

0b65da8

Now uses "emit on destroy" pattern for stepper to subscribe to step changes (fixes angular#8397)

Update stepper.ts

e20ebe6

Removed (now) unneccessary Subscription import

Update stepper.ts

238d5ce

Update stepper.ts

dd32b69

Moved comment to appropriate position

4cfe545

Update stepper.ts

bda41ee

Fixed missing import

Update stepper.ts

dd7b540

Update stepper.ts

7fa8502

Update stepper.ts

c6bde9e

actra-gschuster added 2 commits November 18, 2017 10:36

Update stepper.ts

1bc4b59

Update stepper.ts

6ef5712

jelbourn force-pushed the actra-gschuster-patch-1 branch from 947614f to 6ef5712 Compare November 18, 2017 18:39

jelbourn merged commit 2bc0b41 into angular:master Nov 20, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force change-detection upon ContentChildren()... #8398

Force change-detection upon ContentChildren()... #8398

actra-gschuster commented Nov 12, 2017

mmalerba Nov 14, 2017

actra-gschuster Nov 14, 2017

mmalerba Nov 14, 2017

mmalerba Nov 14, 2017

actra-gschuster commented Nov 15, 2017 •

edited

Loading

willshowell Nov 15, 2017

actra-gschuster Nov 15, 2017

willshowell Nov 15, 2017

actra-gschuster Nov 15, 2017

mmalerba left a comment

mmalerba Nov 15, 2017

actra-gschuster commented Nov 15, 2017

mmalerba commented Nov 15, 2017

mmalerba commented Nov 16, 2017

actra-gschuster commented Nov 17, 2017

willshowell commented Nov 17, 2017

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

Force change-detection upon ContentChildren()... #8398

Force change-detection upon ContentChildren()... #8398

Conversation

actra-gschuster commented Nov 12, 2017

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

actra-gschuster commented Nov 15, 2017 • 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

mmalerba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

actra-gschuster commented Nov 15, 2017

mmalerba commented Nov 15, 2017

mmalerba commented Nov 16, 2017

actra-gschuster commented Nov 17, 2017

willshowell commented Nov 17, 2017

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

actra-gschuster commented Nov 15, 2017 •

edited

Loading