Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
May 25, 2017

Conversation

tinayuangao
Copy link
Contributor

Fixes #2491

R: @jelbourn

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 4, 2017
@tinayuangao tinayuangao changed the title Make MdRadioButton change detection strategy OnPush fix(radio): Make MdRadioButton change detection strategy OnPush Jan 4, 2017
}
set labelPosition(v: 'before' | 'after') {
this._labelPosition = v
if (this._radios) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@jelbourn
Copy link
Member

jelbourn commented May 1, 2017

@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 tinayuangao force-pushed the radio-button-update branch 2 times, most recently from 7178df1 to 0ccedce Compare May 8, 2017 18:09
@tinayuangao
Copy link
Contributor Author

Synced and added some comments

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Something like _markRadiosForCheck

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

_groupValueChanged() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@tinayuangao poke on this one 👈

@tinayuangao
Copy link
Contributor Author

Comments addressed. Please take another look. Thanks!

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a last couple of comments.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call _markRadiosForCheck() here?

}
}

constructor(private _change: ChangeDetectorRef) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tinayuangao tinayuangao added the action: merge The PR is ready for merge by the caretaker label May 23, 2017
@tinayuangao tinayuangao merged commit 97a9bdc into angular:master May 25, 2017
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

md-radio with push strategy not updateing
5 participants