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
base: master
from

Conversation

Projects
None yet
5 participants
@kara
Contributor

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.
@Input()
set value(value: any) {
if (this._select == null) return;
this.id = this._select._addOption(this.id, value);

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

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.

registerOnTouched(fn: () => any): void { this.onTouched = fn; }
_addOption(id: string, value: any): string {
if (!isPresent(id)) id = (this._idCounter++).toString();

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

Remove the if statement. We should always generate a new id.

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

Just a thought. Have you considered doing something like ${value}_${this._idCounter++}? You know, so the values are a bit more meaningful.

This comment has been minimized.

@kara

kara Mar 31, 2016

Contributor

Yeah, I was thinking about that. But what if the value is a huge object? Would we really want a 100 char string in the value attribute? We could maybe enforce a char limit and cut it off to prevent this...

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

Yes, enforcing a char limit sounds ok

fixture.detectChanges();
tick();
ListWrapper.insert(testComp.data, 2, {"name": "Buffalo"});

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

Use testComp.data.push

This comment has been minimized.

@kara

kara Mar 31, 2016

Contributor

That is what I had originally, but it failed in Dart tests.

Error:
Untyped property access to "push" which could be a special ts2dart builtin

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

you just need to type it

This comment has been minimized.

@kara

kara Mar 31, 2016

Contributor

OK, that worked, thanks!

var buffalo = fixture.debugElement.queryAll(By.css("option"))[1];
expect(select.nativeElement.value).toEqual("1");
expect(buffalo.nativeElement.selected).toBe(true);

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

I think you need to assert a bit more here. Maybe check that it is actually Buffalo and not NYC?

This comment has been minimized.

@kara

kara Mar 31, 2016

Contributor

You mean the text in the select? I'm not sure there's a way to check that without checking the value property. Its text content is the all the option values.

_removeOption(id: string): void { this._optionMap.delete(id); }
_getOptionId(value: any): string {

This comment has been minimized.

@vsavkin

vsavkin Mar 31, 2016

Contributor

Could you write a test that creates multiple options with the same value? As far as I understand, the first option will be selected. But it might be a good idea to explicitly state it by having a test.

This comment has been minimized.

@kara

kara Mar 31, 2016

Contributor

Good call, I can add one. While I'm at it, maybe I can add one for objects with the same content but different identities...

@kara

This comment has been minimized.

Contributor

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;

This comment has been minimized.

@vsavkin

vsavkin Apr 1, 2016

Contributor

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

This comment has been minimized.

@kara

kara Apr 1, 2016

Contributor

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.

This comment has been minimized.

@vsavkin

vsavkin Apr 1, 2016

Contributor

Use looseIdentical

}
_buildValueString(id: string, value: any): string {
if (isStringMap(value)) value = "Object";

This comment has been minimized.

@vsavkin

vsavkin Apr 1, 2016

Contributor

Even though it will work, it is not a good idea to use isStringMap here because of Dart. Use !isPrimitive instead.

_extractId(valueString: string): string {
var delimiter = RegExpWrapper.create(":");
return StringWrapper.split(valueString, delimiter)[0];

This comment has been minimized.

@vsavkin

vsavkin Apr 1, 2016

Contributor

Just use valueString.split(":")

fixture.detectChanges();
tick();
testComp.list[1] = {"name": "Buffalo"};

This comment has been minimized.

@vsavkin

vsavkin Apr 1, 2016

Contributor

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

set value(value: any) {
if (this._select == null) return;
this._select._optionMap.set(this.id, value);
this._setElementValue(this._select._buildValueString(this.id, value));

This comment has been minimized.

@vsavkin

vsavkin Apr 1, 2016

Contributor

Totally up to you. But if you find accessing _buildValueString via _select awkward, you can make a standalone function.

this._renderer.setElementProperty(this._element.nativeElement, 'value', value);
}
ngOnDestroy() {

This comment has been minimized.

@vsavkin

vsavkin Apr 1, 2016

Contributor

I might be mistaken, but I think you need to call _select.writeValue here. And add a test.

This comment has been minimized.

@kara

kara Apr 1, 2016

Contributor

You're right, added

@jeffbcross

This comment has been minimized.

Contributor

jeffbcross commented Apr 1, 2016

@jeffbcross jeffbcross assigned kara and unassigned vsavkin Apr 1, 2016

@kara

This comment has been minimized.

Contributor

kara commented Apr 2, 2016

@jeffbcross Fixed the Edge issues. Should be ready!

@mary-poppins

This comment has been minimized.

mary-poppins commented Apr 6, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment