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(select): support objects as select values #7842

Closed
wants to merge 1 commit into from

Conversation

kara
Copy link
Contributor

@kara kara commented Mar 31, 2016

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
  • Tests for the changes have been added (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    This fixes a current bug where objects set as option values and/or [(ngModel)] on select tags are converted to [object Object] strings.
  • What is the current behavior?

For the following:

@Component({...})
class MyComp {
   selected: any;
   options: any[] = [
      {name: 'Pepper'},
      {name: 'Salt'},
      {name: 'Paprika'}
   ];

   constructor() {  this.selected = this.options[1]; }
}
<select [(ngModel)]="selected">
   <option *ngFor="#current of options" [value]="current">{{current.name}}</option>
</select>

You might expect the select to display "Salt", but it displays "Pepper" because all option values are set to [object Object]. When selecting new options using the dropdown, the selected property on the component class is set to [object Object].

This PR allows you to write the above code and have the selection updated on both ends as expected.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No breaking changes.

@kara kara force-pushed the select branch 8 times, most recently from 1609c57 to 8b73e3b Compare March 31, 2016 18:19
@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Mar 31, 2016
@Input()
set value(value: any) {
if (this._select == null) return;
this.id = this._select._addOption(this.id, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you move it to the constructor?

Since id never changes, it is a bit confusing that you keep resetting it on every value change.

@kara kara force-pushed the select branch 5 times, most recently from 45eaaf9 to 6e017f6 Compare April 1, 2016 00:07
@kara
Copy link
Contributor Author

kara commented Apr 1, 2016

@vsavkin All comments should be addressed.


_getOptionId(value: any): string {
for (let id of MapWrapper.keys(this._optionMap)) {
if (this._optionMap.get(id) == value) return id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean to use == or ===? Those two things are somewhat distinct in JS, and are very distinct in Dart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did mean == to fix a Dart error, but thinking about it more, this is a bad solution. Obviously we don't want 6 to match "6". I'll fix it another way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use looseIdentical

fixture.detectChanges();
tick();

testComp.list[1] = {"name": "Buffalo"};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I can remove this line, and the test will still pass. Can you verify that the selected option is actually Buffalo?

@kara kara force-pushed the select branch 14 times, most recently from 187aea6 to aa05492 Compare April 2, 2016 02:03
@kara kara added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 2, 2016
@kara
Copy link
Contributor Author

kara commented Apr 2, 2016

@jeffbcross Fixed the Edge issues. Should be ready!

@mary-poppins
Copy link

Merging PR #7842 on behalf of @vsavkin to branch presubmit-vsavkin-pr-7842.

@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 8, 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 area: forms cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants