fix(selection-list): fix option value coercion and selection events #6901

dereekb · 2017-09-07T08:09:01Z

md-list-option value is no longer coerced to a boolean value
md-list-option EventEmitters selectedChange and deselected now emit an event when an option is selected/deselected

willshowell · 2017-09-07T10:27:17Z

src/lib/list/selection-list.ts

@@ -97,7 +97,7 @@ export class MdListOption extends _MdListOptionMixinBase

  @Input()
  get value() { return this._value; }
-  set value( val: any) { this._value = coerceBooleanProperty(val); }
+  set value( val: any) { this._value = val; }


The getter and setter can be removed if you're no longer coercing

rafaelss95 · 2017-09-07T16:12:11Z

I have some statements about this component.

1 - Why is there an event for select and also for deselect? Can't it be just one event, like in checkbox?

2 - Rename the interface(class - see below) from MdSelectionListOptionEvent to MdListOptionChange to maintain consistency.

3 - Shouldn't SelectionOptionEvent be a class instead of interface to maintain the consistency? And also return the properties checked and source as usual.

Actual:

export interface MdListOptionChange {
  option: MdListOption;
}

...

this.selectChange.emit({option: this});

Proposal (similar to checkbox):

export class MdListOptionChange {
  checked: boolean;
  source: MdListOption;
}

...
const event = new MdListOptionChange();
event.checked = this.selected;
event.source = this;

this.selectChange"D".emit(event);

4 - Is the (destroyed) useful at some point?

Actual:

  ngOnDestroy(): void {
    this.destroyed.emit({option: this});
  }

--

5 - Shouldn't onFocus be decorated with @Output?

Actual:
onFocus = new EventEmitter<MdSelectionListOptionEvent>();

Proposal:
@Output() onFocus = new EventEmitter<MdSelectionListOptionEvent>();

jelbourn · 2017-09-07T16:18:18Z

Yeah, that's a good point. It should just have one selectionChanged event where the event includes the new state.

dereekb · 2017-09-07T16:22:45Z

In my current project I am watching the destroyed event so I can remove options that are no longer available. You could probably use the changes QueryList in selectionList, but you'd have to figure out which ones were removed.

Looking at the Material code again, the MdSelectionList itself subscribes to the destroyed events to figure out which option to focus to once one is destroyed.

I haven't used enough elements to see one destroyed yet, though.

rafaelss95 · 2017-09-07T17:21:44Z

@jelbourn what do you think about other statements?

jelbourn · 2017-09-07T17:28:41Z

@rafaelss95 those should be covered under #6677

dereekb · 2017-09-07T17:32:17Z

I went ahead and pushed changes that were relevant to the original issue.

I didn't rename MdSelectionListOptionEvent, but I changed it to a class now so it stayed consistent with similar classes.

I skipped the focus issue, but it should probably be a Subject instead of an EventEmitter looking at other classes like MdChip where it is only used internally.

  /** Emits when the chip is focused. */
  _onFocus = new Subject<MdChipEvent>();

jelbourn

LGTM

paveltretyakovru · 2017-09-11T09:15:39Z

Hello, Everybody!
When are you merge the pull request?
Thank you for your work :)

mmalerba · 2017-09-11T22:54:47Z

please rebase

dereekb · 2017-09-11T23:16:51Z

rebase with master or _presubmit? Looks like someone else made some of the the same changes.

(probably a dumb question but I don't do this often. I'm working on rebasing with master right now.)

Edit: Rebased it with master. Everything looked ok

dereekb · 2017-09-11T23:32:46Z

Scratch that, I broke it. Resolved the conflict the wrong way and erased the changes made to selection-list! I see master has moved some more so I'll rebase again and not destroy the changes this time. 😄

dereekb · 2017-09-12T02:31:33Z

Ok, rebased with changes #6983 and #6971 included.

kara · 2017-10-03T17:40:40Z

@dereekb Would you mind rebasing one more time?

dereekb · 2017-10-03T18:03:56Z

Sure.

I'm rebasing right now, but it looks like a lot of the changes in this pull request are already on master one way or another from other pull requests.

The only changes are changing MatSelectionListOptionEvent to a class, and merging selectChange and deselected together into a single event emitter, that last one being a breaking change.

dereekb · 2017-10-10T17:14:00Z

Noticed I need to rebase it again. Working on that now.

…event emitters * md-list-option value is no longer coerced to a boolean value * md-list-option EventEmitters selectedChange and deselected now emit an event when an option is selected/deselected Fixes #6864

dereekb · 2017-10-10T18:52:46Z

Ok, rebased and all the unit tests passed on my end.

I want to bring up a slight change from this rebase:

I removed _selected and instead have the getter use the selectionList's selectedOptions isSelected() for whether or not the object is selected.

My reasoning is that the selection's state shouldn't be in two places (by this I mean MatSelectionList and other components check selectedOptions for is selected, while the MatListOption checks itself), and the performance change is minimal since a Set is used for selectedOptions.

Another thing, since selectedOptions in MatSelectionList is publicly accessible, externally modifying selectedOptions should also send selectionChange events on the MatListOption right? I don't think it would, so it's something to consider.

adarowiec · 2017-10-12T09:30:37Z

Can you guys already pull it? I really need this fix in my project.

rafaelss95 · 2017-10-12T16:55:06Z

src/lib/list/selection-list.spec.ts

@@ -579,3 +658,28 @@ class SelectionListWithTabindexBinding {
  tabIndex: number;
  disabled: boolean;
 }
+
+@Component({template: `
+<mat-selection-list id = "selection-list-5">


remove the spaces around: id="selection-list-5"?

I don't know if I hit the format button on my side, but there are spaces around most of the unit test component's id's in this file.

I'll fix them.

You're right. There are a lot of strange spaces in this spec file.

rafaelss95 · 2017-10-12T16:56:33Z

src/lib/list/selection-list.ts

@@ -174,6 +169,16 @@ export class MatListOption extends _MatListOptionMixinBase
    this.selectionList._setFocusedOption(this);
  }

+  /** Creates a selection event object from the specified option. */
+  private _createChangeEvent(option: MatListOption = this): MatSelectionListOptionEvent {
+    let event = new MatSelectionListOptionEvent();


rafaelss95 · 2017-10-12T17:04:10Z

src/lib/list/selection-list.ts

  option: MatListOption;
+  selected: boolean;
 }


I'd keep the consistency between this and other components:

/** Change event object emitted by MatListOption */ export class MatListOptionChange { /** The source MatListOption of the event. */ source: MatListOption; /** The new `selected` value of the option. */ selected: boolean; }

See checkbox for example.

So checked instead of selected? Gotcha.

In fact, all changes above (to keep consistency). Also with comments so it can be shown in docs.

By others you mean other checkboxes? Add comments?

Changing it from selected to checked seems like a step in the wrong direction, since it is MatListOption.selected, not MatListOption.checked.

You're right (bad copy). Comments === docs description.

What about source instead of option? All change events use source?
Also, MatListOptionChange instead of MatSelectionListOptionEvent?

I updated the code above.

Sounds good, I'll change that.

rafaelss95 · 2017-10-12T17:10:54Z

src/lib/list/selection-list.ts

  /** Whether the option is selected. */
  @Input()
-  get selected() { return this._selected; }
+  get selected() { return this.selectionList.selectedOptions.isSelected(this); }


Can you add :boolean here? get selected(): boolean.

rafaelss95 · 2017-10-12T17:25:05Z

src/lib/list/selection-list.ts

@@ -97,34 +97,29 @@ export class MatListOption extends _MatListOptionMixinBase
  /** Whether the label should appear before or after the checkbox. Defaults to 'after' */
  @Input() checkboxPosition: 'before' | 'after' = 'after';

-  /** Value of the option */
-  @Input() value: any;
-
  /** Whether the option is disabled. */
  @Input()
  get disabled() { return (this.selectionList && this.selectionList.disabled) || this._disabled; }


: boolean?

rafaelss95 · 2017-10-12T17:25:17Z

src/lib/list/selection-list.ts

@@ -97,34 +97,29 @@ export class MatListOption extends _MatListOptionMixinBase
  /** Whether the label should appear before or after the checkbox. Defaults to 'after' */
  @Input() checkboxPosition: 'before' | 'after' = 'after';

-  /** Value of the option */
-  @Input() value: any;
-
  /** Whether the option is disabled. */
  @Input()
  get disabled() { return (this.selectionList && this.selectionList.disabled) || this._disabled; }
  set disabled(value: any) { this._disabled = coerceBooleanProperty(value); }


: boolean?

~~Value is the actual selection option value.~~

~~We don't want to coerce it to a boolean, again! 😛~~

Edit: Misread

Well... for selected we have: set selected(value: boolean) { const isSelected = coerceBooleanProperty(value); ...

Gotcha, fixed.

Ah, ok. The highlighted code I see is both on this section:

/** Value of the option */ - @Input() value: any;

So I was kind of guessing what you meant. Making those changes now.

rafaelss95 · 2017-10-12T17:46:04Z

src/lib/list/selection-list.ts

@@ -111,7 +111,7 @@ export class MatListOption extends _MatListOptionMixinBase
  @Input()
  get selected(): boolean { return this.selectionList.selectedOptions.isSelected(this); }
  set selected(value: boolean) {
-    const isSelected = coerceBooleanProperty(value);
+    const isSelected: boolean = coerceBooleanProperty(value);


I think you misunderstood what I mean... I mean that we have the following signature for selected: set selected(value: boolean) { ... = coerceBooleanProperty(value); }, so the disabled should follow the same thing.

Basically change the signature of disabled from set disabled(value: any) { to set disabled(value: boolean) {.

Did you get it now?

This :boolean added here is unnecessary.

Yea, the highlighted section GitHub was showing was:

/** Value of the option */ - @Input() value: any;

So I didn't really get what you were talking about. It's changed now and I removed the unnecessary :boolean.

Aha, ok. Nice :)

Will you remove this :boolean in another commit?

That's the second time today I've committed without first hitting save. Done.

rafaelss95 · 2017-10-12T17:48:17Z

src/lib/list/selection-list.ts

@@ -51,16 +51,18 @@ export const _MatSelectionListMixinBase =
 export class MatListOptionBase {}
 export const _MatListOptionMixinBase = mixinDisableRipple(MatListOptionBase);

-/** Event emitted by a selection-list whenever the state of an option is changed. */
+/** Change event object emitted by MatListOption */
 export class MatSelectionListOptionEvent {


MatListOptionChange?

andrewseguin · 2017-10-12T20:41:17Z

Can you check on the tests and get them green?

dereekb · 2017-10-12T20:45:14Z

The tests that failed aren't related to the changes (they have to do with MatAutocomplete).

Edge 14.14393.0 (Windows 10 0.0.0) MatAutocomplete forms integration should disable input in view when disabled programmatically FAILED
Expected false to be true, 'Expected input underline to display disabled styles.'.

Those failed tests are related to (and will be fixed by) #7743, I think.

alisonmsmith · 2017-10-24T18:20:54Z

FYI - material docs specify the event @Output selectChange which is of type EventEmitter<MatSelectionListOptionEvent> and instead it looks like it is implemented here as @Output selectionChange of type EventEmitter<MatListOptionChange> also the @Output deselected from the docs is still not emitted (although you can check if the option was selected or deselected from the MatListOptionChange boolean selected property)

dereekb · 2017-10-24T21:12:52Z

Yea, the docs (probably) need to be updated to reflect these changes.

selected/deselected were removed and replaced with selectionChange, since having a single selectionChange is more consistent in terms of the rest of Angular Material implementations.

As for the selectChange/selectionChange name, there was another discussion thread where I believe selectionChange was picked as the more desirable. I don't see the reference in this thread anymore, but it was about naming consistency and updating across a number of different Material components.

angular-automatic-lock-bot · 2019-09-07T11:02:47Z

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

dereekb requested review from crisbeto and jelbourn as code owners September 7, 2017 08:09

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

willshowell reviewed Sep 7, 2017

View reviewed changes

jelbourn approved these changes Sep 7, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 7, 2017

rafaelss95 mentioned this pull request Sep 11, 2017

fix(selection-list): do not coerece option value to boolean #6983

Merged

mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 11, 2017

mmalerba added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Sep 12, 2017

willshowell mentioned this pull request Sep 13, 2017

chore(chips): update API per #6677 #7030

Closed

josephperrott mentioned this pull request Sep 28, 2017

[MdListOption] selectChange/deselected events don't emit #7396

Closed

andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 29, 2017

kara assigned dereekb Oct 3, 2017

charlesrollin mentioned this pull request Oct 4, 2017

md-list-option in md-selection-list not calling selectChange and deselect when toggled #7529

Closed

josephperrott mentioned this pull request Oct 4, 2017

bug(MdListOption/MdSelectionList) selectChange does not fire on MdListOption #7533

Closed

fix(selection-list): fix md-list-option value coercion and selection …

eb3f6da

…event emitters * md-list-option value is no longer coerced to a boolean value * md-list-option EventEmitters selectedChange and deselected now emit an event when an option is selected/deselected Fixes #6864

mmalerba added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Oct 12, 2017

rafaelss95 reviewed Oct 12, 2017

View reviewed changes

dereekb added 2 commits October 12, 2017 12:38

review feedback changes

390cb1d

review feedback changes

801c92c

rafaelss95 reviewed Oct 12, 2017

View reviewed changes

dereekb added 2 commits October 12, 2017 12:56

review feedback changes

bc14f66

review feedback changes

efab749

andrewseguin removed the action: merge The PR is ready for merge by the caretaker label Oct 12, 2017

andrewseguin merged commit 80671bf into angular:master Oct 12, 2017

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

fix(selection-list): fix option value coercion and selection events #6901

fix(selection-list): fix option value coercion and selection events #6901

Conversation

dereekb commented Sep 7, 2017

Choose a reason for hiding this comment

rafaelss95 commented Sep 7, 2017 • edited Loading

jelbourn commented Sep 7, 2017

dereekb commented Sep 7, 2017 • edited Loading

rafaelss95 commented Sep 7, 2017

jelbourn commented Sep 7, 2017

dereekb commented Sep 7, 2017 • edited Loading

jelbourn left a comment

Choose a reason for hiding this comment

paveltretyakovru commented Sep 11, 2017

mmalerba commented Sep 11, 2017

dereekb commented Sep 11, 2017 • edited Loading

dereekb commented Sep 11, 2017 • edited Loading

dereekb commented Sep 12, 2017

kara commented Oct 3, 2017

dereekb commented Oct 3, 2017

dereekb commented Oct 10, 2017

dereekb commented Oct 10, 2017 • edited Loading

adarowiec commented Oct 12, 2017

rafaelss95 Oct 12, 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

rafaelss95 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelss95 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelss95 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelss95 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereekb Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

rafaelss95 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelss95 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

dereekb Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dereekb Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewseguin commented Oct 12, 2017

dereekb commented Oct 12, 2017 • edited Loading

alisonmsmith commented Oct 24, 2017 • edited Loading

dereekb commented Oct 24, 2017 • edited Loading

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

rafaelss95 commented Sep 7, 2017 •

edited

Loading

dereekb commented Sep 7, 2017 •

edited

Loading

dereekb commented Sep 7, 2017 •

edited

Loading

dereekb commented Sep 11, 2017 •

edited

Loading

dereekb commented Sep 11, 2017 •

edited

Loading

dereekb commented Oct 10, 2017 •

edited

Loading

rafaelss95 Oct 12, 2017 •

edited

Loading

rafaelss95 Oct 12, 2017 •

edited

Loading

rafaelss95 Oct 12, 2017 •

edited

Loading

rafaelss95 Oct 12, 2017 •

edited

Loading

rafaelss95 Oct 12, 2017 •

edited

Loading

dereekb Oct 12, 2017 •

edited

Loading

rafaelss95 Oct 12, 2017 •

edited

Loading

rafaelss95 Oct 12, 2017 •

edited

Loading

dereekb Oct 12, 2017 •

edited

Loading

dereekb Oct 12, 2017 •

edited

Loading

dereekb commented Oct 12, 2017 •

edited

Loading

alisonmsmith commented Oct 24, 2017 •

edited

Loading

dereekb commented Oct 24, 2017 •

edited

Loading