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

[MdListOption] value always boolean, selectChange/deselected events don't fire #6864

Closed
dereekb opened this issue Sep 6, 2017 · 11 comments · Fixed by #6901
Closed

[MdListOption] value always boolean, selectChange/deselected events don't fire #6864

dereekb opened this issue Sep 6, 2017 · 11 comments · Fixed by #6901

Comments

@dereekb
Copy link
Contributor

dereekb commented Sep 6, 2017

Bug, feature request, or proposal:

MdListOption's output EventEmitters selectChange, and deselected do not fire when the item is selected/deselected.

What is the expected behavior?

They should activate the angular expression bound to them.

What is the current behavior?

Here's the code block I'm working with:

<md-selection-list class="selection-content-list">
    <md-list-option class="list-item" (selectChange)="selectedItem($event)" (deselected)="deselectedItem($event)" [disabled]="selectionItem.disabled" *ngFor="let selectionItem of (elements | async)">
        <md-icon mdListIcon>folder</md-icon>
        <h4 mdLine>{{ selectionItem.name }}</h4>
    </md-list-option>
</md-selection-list>

The items visually update when being clicked on and off.
The MdSelectionList's selectedOptions variable updates properly.
The selectChange and deselected EventEmitters are never activated.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Angular Material 2.0.0-beta.10

Is there anything else we should know?

Looking at the current code, it looks like they aren't ever used or references apart from their declarations.

The focus and destroyed events seem to emit fine though.

Also the value variable for MdListOption is coerced to a boolean value so even using selectedOptions doesn't make the MdListOption useable.

@julianobrasil
Copy link
Contributor

A plunk is always useful: https://plnkr.co/edit/6y5tPM?p=preview

@dereekb
Copy link
Contributor Author

dereekb commented Sep 6, 2017

Thanks! I updated your plunk to show the two issues, the second issue being all values being coerced to a boolean.

https://plnkr.co/edit/hfo1kUlv7zxGioL1y72C?p=preview

The second issue probably belongs in it's own bug report, but its a 1 line fix in selection-list.ts:

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

The EventEmitter fix is also straight forward:

...
  toggle(): void {
    this.selected = !this.selected;
    this.selectionList.selectedOptions.toggle(this);
    this._changeDetector.markForCheck();

    if (this.selected) {
      this.selectChange.emit({option: this});
    } else {
      this.deselected.emit({option: this});
    }
  }
...

When I get a chance (and figure out how) I'll submit a pull request for that/those fixes.

@julianobrasil
Copy link
Contributor

julianobrasil commented Sep 6, 2017

If it's intended to work like checkboxes, the value will always be a boolean. The actual value would be accessible via the selectChange method's $event.

Edit: No, it's not like this. I get a little bit confused with this once in a while: if you bind a component variable to ngModel, it get the checkbox state instead of it's value. Nothing to do to what is being passed to onChange observer method.

@dereekb
Copy link
Contributor Author

dereekb commented Sep 6, 2017

Yea, I think someone just made a copy/paste typo/error. The value's type is set to "any" but it gets coerced just like the setter below it:

 private _selected: boolean = false;
 private _value: any;

...

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

 @Input()
 get selected() { return this._selected; }
 set selected( val: boolean) { this._selected = coerceBooleanProperty(val); }

As I said before this is a separate issue that probably belongs in it's own bug report since it effectively makes the Selection List a checkbox.

I'll make a pull request when I get a chance, unless someone else beats me to it.

@dereekb dereekb changed the title [MdListOption] selectChange/deselected events don't fire [MdListOption] value always boolean, selectChange/deselected events don't fire Sep 6, 2017
@glampr
Copy link

glampr commented Sep 18, 2017

I've also noticed that when a MdListOption is already selected when it initially loads (has an @input [selected]="true") then its MdSelectionList.selectedOptions.selected are incorrectly populated, i.e. it seems to add the options to the selected list when it is clicked and becomes deselected.

@dereekb Have you noticed anything similar and is the issue I'm describing something that will be fixed with the changes in you PR?

@dereekb
Copy link
Contributor Author

dereekb commented Sep 18, 2017

@glampr I do recall running into that issue come to think of it and don't think there is a unit test for that case.

In my case I was already making a specific controller/wrapper for MdSelectionList so I worked around it by setting the values after they were added to the views to avoid running into that issue, but initially I did try to set selected through the *ngFor loop and initially selected items weren't working like you described.

When I get a chance I'll add a new unit test to test against it. Is there a bug report for it?

@glampr
Copy link

glampr commented Sep 19, 2017

@dereekb as far as I can tell there is no bug report for this.
Do you want me to create a separate issue?

@dereekb
Copy link
Contributor Author

dereekb commented Sep 19, 2017

Yea, I think that would be a good idea since this issue has been resolved.

@glampr
Copy link

glampr commented Sep 19, 2017

@dereekb created issue #7183 just in case you want to follow it.

@josephperrott
Copy link
Member

Closing as this issue is resolved as noted above.

andrewseguin pushed a commit that referenced this issue Oct 12, 2017
…6901)

* fix(selection-list): fix md-list-option value coercion and selection 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

* review feedback changes

* review feedback changes

* review feedback changes

* review feedback changes
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants