fix(radio): Make MdRadioButton change detection strategy OnPush #2526

tinayuangao · 2017-01-04T20:17:38Z

jelbourn · 2017-01-04T21:49:38Z

src/lib/radio/radio.ts

+  }
+  set labelPosition(v: 'before' | 'after') {
+    this._labelPosition = v
+    if (this._radios) {


This shouldn't be necessary; the individual radios have a getter that reads the value from the group if it exists without clobbering its own value.

jelbourn · 2017-01-04T21:51:53Z

src/lib/radio/radio.ts

+  }
+  set ariaLabel(value: string) {
+    this._ariaLabel = value;
+    this._change.markForCheck();


I didn't think calling markForCheck for every property shouldn't be necessary; for anything that is an @Input Angular will do change detection as needed when the value is changed via a template binding. The only thing I know of that needs to change is that the writeValue function needs to call markForCheck, since the template binding is on ngModel (which isn't an @Input of the component).

Then there's a bug for OnPush since the properties are not updated when their values changed.

The writeValue function is in MdRadioButtonGroup which is a directive and has no changeDetection strategy.

@jelbourn your understanding is correct, an @Input() property updated via a binding would mark the components for change detection. (it would not if it accessed directly, ie myDirective.inputProperty = ... in some component).

@tinayuangao

Then there's a bug for OnPush since the properties are not updated when their values changed.

I'm not sure to get what you mean here.

Only components are nodes in the change detection tree. Directives are checked by the CD together with their component and I expect that setting any of their input would have the same effect as setting a component input - I have to double check this case.

For labelPosition and disabled, group buttons' values depend on group's values. When the group's disabled changed, the button's get disabled also changed but set disabled is not called and button's _disabled is not changed. We need to let buttons markForCheck() when the group value changed. I added a _groupValueChanged() function but not sure it's the right way to do.

jelbourn · 2017-01-12T01:56:15Z

@vicb Is this the way OnPush is supposed to work right now?

jelbourn · 2017-05-01T22:20:15Z

@tinayuangao I'm trying to clean up old PRs; can you rebase this?

Revisiting this PR, I think what's missing is, for each markForCheck, we should add a comment that explains what in particular is changing and/or why markForCheck is necessary

tinayuangao · 2017-05-08T18:11:39Z

Synced and added some comments

jelbourn · 2017-05-10T23:11:19Z

src/lib/radio/radio.ts

  set disabled(value: boolean) {
    this._disabled = coerceBooleanProperty(value);
+    // Update rippleDisabled
+    this._changeDetector.markForCheck();


Could you elaborate on what this comment means? I don't see why you would need this in the setter since disabled is an @Input

jelbourn · 2017-05-10T23:15:47Z

src/lib/radio/radio.ts

+    this._labelPosition = (v == 'before') ? 'before' : 'after';
+    if (this._radios) {
+      this._radios.forEach(radio => radio._groupValueChanged());
+    }


I think this part should be broken out into its own function

if (this._radios) { this._radios.forEach(radio => radio._groupValueChanged()); }

Something like _markRadiosForCheck

jelbourn · 2017-05-10T23:18:17Z

src/lib/radio/radio.ts

@@ -427,6 +466,12 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
    this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'keyboard');
  }

+  _groupValueChanged() {


I would just call this function _markForCheck with a comment like

/** * Marks the radio button as needing checking for change detection. * This method is exposed because the parent radio group will directly * update bound properties of the radio button. */

jelbourn · 2017-05-23T17:00:43Z

@tinayuangao poke on this one 👈

tinayuangao · 2017-05-23T18:38:15Z

Comments addressed. Please take another look. Thanks!

jelbourn

LGTM, just a last couple of comments.

Add the "merge_ready" label when it's ready for submit

jelbourn · 2017-05-23T18:41:05Z

src/lib/radio/radio.ts

+    this._disabled = value;
+    if (this._radios) {
+      // Update radios disabled state
+      this._radios.forEach((r) => r._markForCheck());


Call _markRadiosForCheck() here?

jelbourn · 2017-05-23T18:41:54Z

src/lib/radio/radio.ts

+    }
+  }
+
+  constructor(private _change: ChangeDetectorRef) {


I would call this _changeDetector, since _change could be confused with the change event

angular-automatic-lock-bot · 2019-09-06T13:01:48Z

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

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

tinayuangao requested a review from jelbourn January 4, 2017 20:17

tinayuangao changed the title ~~Make MdRadioButton change detection strategy OnPush~~ fix(radio): Make MdRadioButton change detection strategy OnPush Jan 4, 2017

jelbourn reviewed Jan 4, 2017

View reviewed changes

jelbourn assigned vicb Jan 13, 2017

tinayuangao force-pushed the radio-button-update branch 2 times, most recently from ec34eb8 to e257577 Compare January 18, 2017 18:49

andrewseguin added the pr: needs rebase label Mar 2, 2017

tinayuangao force-pushed the radio-button-update branch from e257577 to 1ca794f Compare March 2, 2017 22:12

tinayuangao removed the pr: needs rebase label Mar 2, 2017

tinayuangao force-pushed the radio-button-update branch 2 times, most recently from 6089d3a to 7f4c885 Compare April 11, 2017 01:42

tinayuangao force-pushed the radio-button-update branch 2 times, most recently from 7178df1 to 0ccedce Compare May 8, 2017 18:09

tinayuangao force-pushed the radio-button-update branch from 0ccedce to ea16bbc Compare May 9, 2017 17:23

jelbourn reviewed May 10, 2017

View reviewed changes

jelbourn added the pr: needs review label May 23, 2017

tinayuangao added 8 commits May 23, 2017 10:44

Make MdRadioButton OnPush

e73ad6b

.

44ef4c0

Fixed tests

7026609

fixed group ripple disabled test

6691443

fix lint

7b0cfec

fix test

96fb7d7

sync and fix test

e1a3618

address comments

0e4dfdc

tinayuangao force-pushed the radio-button-update branch from ea16bbc to 0e4dfdc Compare May 23, 2017 18:37

jelbourn approved these changes May 23, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels May 23, 2017

jelbourn unassigned vicb May 23, 2017

address comments

f7ec787

tinayuangao added the action: merge The PR is ready for merge by the caretaker label May 23, 2017

tinayuangao merged commit 97a9bdc into angular:master May 25, 2017

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(radio): Make MdRadioButton change detection strategy OnPush #2526

fix(radio): Make MdRadioButton change detection strategy OnPush #2526

tinayuangao commented Jan 4, 2017

jelbourn Jan 4, 2017

jelbourn Jan 4, 2017

tinayuangao Jan 5, 2017

vicb Jan 13, 2017

tinayuangao Jan 18, 2017

jelbourn commented Jan 12, 2017

jelbourn commented May 1, 2017

tinayuangao commented May 8, 2017

jelbourn May 10, 2017

jelbourn May 10, 2017

jelbourn May 10, 2017

jelbourn commented May 23, 2017

tinayuangao commented May 23, 2017

jelbourn left a comment

jelbourn May 23, 2017

jelbourn May 23, 2017

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

fix(radio): Make MdRadioButton change detection strategy OnPush #2526

fix(radio): Make MdRadioButton change detection strategy OnPush #2526

Conversation

tinayuangao commented Jan 4, 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

Choose a reason for hiding this comment

jelbourn commented Jan 12, 2017

jelbourn commented May 1, 2017

tinayuangao commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented May 23, 2017

tinayuangao commented May 23, 2017

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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