Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

docs(ngOptions): explain using 'select as' and 'track by' together #13007

Closed
wants to merge 1 commit into from
Closed

docs(ngOptions): explain using 'select as' and 'track by' together #13007

wants to merge 1 commit into from

Conversation

ryanhart2
Copy link
Contributor

  • change warning to indicate that 'select as' and 'track by' can be used together with care
  • provide an example that will work and an example that will not work
  • provide a simple explanation of why the non-working example does not work

Fixes #11768

@gkalpak
Copy link
Member

gkalpak commented Oct 5, 2015

I (obviously) like documenting that it is possible to use select as and track by together, but I also kind of liked the lengthier explanation. I'd prefer a more detailed explanation (not necessarily copy-pasting the old one, but maybe incorporating parts of it).

@petebacondarwin
Copy link
Contributor

Thanks @ryanhart2 and @gkalpak for working on this.
Once you are happy with it, @gkalpak, please merge.

@Narretz Narretz added this to the Backlog milestone Oct 5, 2015
@Narretz
Copy link
Contributor

Narretz commented Oct 8, 2015

I tried to incorporate the original example with the two new examples, but the results are pretty confusing: http://plnkr.co/edit/GBw4YXl3sDzkX8oqXR4s?p=preview

I feel that the example needs to be much clearer in which circumstances the examples will work and in which not. The new "wrong" example works if you only change the selection from th UI, but the problem with the track by expression only occurs when the model is preselected. So this part of the original explanation is completely lost.

I think it would be the best if we had an actual runnable example where you could manipulate the selects and set the model the different values.

* change warning to indicate that 'select as' and 'track by' can be used together with care
* provide an example that will work and an example that will not work
* provide a simple explanation of why the non-working example does not work
@ryanhart2
Copy link
Contributor Author

@gkalpak, @Narretz:
I have updated to incorporate the original example to address your feedback. Do you think this revised version is clearer and correct?

@Narretz Narretz self-assigned this Oct 9, 2015
* selected" option.
* In both examples the **`track by`** expression is applied successfully to each `item` in the `items` array.
* Because the selected option has been set in code, the **`track by`** expression is also applied to the `ngModel`
* value. In the first example, the `ngModel` value is `item[x]` and the **`track by`** expression evaluates to
Copy link
Member

Choose a reason for hiding this comment

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

item[x] --> items[x]

@gkalpak
Copy link
Member

gkalpak commented Oct 13, 2015

@Narretz, wrt #13007 (comment), I don't get what you mean. What is the confusing part ?

I can see the problem happening when the ngModel is set outside of the <select> (either programmatically, or through the UI (e.g. ngClick handler on a button)). Preselected or not doesn't seem to make a difference.

When the value is set via the <select> everything works as expected, because track by is used for "reverse mapping" the model value to an option (and not the other way around).

Do I miss something ?

@gkalpak
Copy link
Member

gkalpak commented Oct 27, 2015

/ping @Narretz
(I'll be merging this soon, so speak up if you have any objections 😃)

@petebacondarwin
Copy link
Contributor

Go for it @gkalpak

@Narretz
Copy link
Contributor

Narretz commented Oct 27, 2015

Sorry for the delay, I'm checking this again, give me 10 minutes.

* a wrong object, the selected element can't be found, `<select>` is always reset to the "not
* selected" option.
* In both examples the **`track by`** expression is applied successfully to each `item` in the `items` array.
* Because the selected option has been set in code, the **`track by`** expression is also applied to the `ngModel`
Copy link
Contributor

Choose a reason for hiding this comment

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

"has been set in code" doesn't really tell me anything. How about "has been set from the scope"?

Copy link
Member

Choose a reason for hiding this comment

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

I'll change it to "has been set programmatically in the controller". How about that ?

@Narretz
Copy link
Contributor

Narretz commented Oct 27, 2015

Apart from a minor nitpick, it's looking good now.

@Narretz Narretz assigned gkalpak and unassigned Narretz Oct 27, 2015
@gkalpak gkalpak closed this in a995ee1 Oct 27, 2015
gkalpak pushed a commit that referenced this pull request Oct 27, 2015
…by` together

Changes:

* Modify warning message to indicate that `track by` can be used with `select as`,
  but subject to certain limitations.
* Provide both a working and an non-working example.
* Explain why the latter does not work.

Closes #13007
@gkalpak
Copy link
Member

gkalpak commented Oct 27, 2015

Tweaked a bit and merged. Also backported to v1.4.x as 980fb39.

👍 to @ryanhart2 for submitting (and to @Narretz and @petebacondarwin for reviewing) !

@Narretz
Copy link
Contributor

Narretz commented Oct 27, 2015

👍 for merging. Technically I don't think there needs to be a
controller involved for setting it programmatically, but a scope does, but
yeah ... it's not terribly important.
Am 27.10.2015 21:08 schrieb "Georgios Kalpakas" [email protected]:

Tweaked a bit and merged. Also backported to v1.4.x as 980fb39
980fb39
.

[image: 👍] to @ryanhart2 https://github.com/ryanhart2 for submitting
(and to @Narretz https://github.com/Narretz and @petebacondarwin
https://github.com/petebacondarwin for reviewing) !


Reply to this email directly or view it on GitHub
#13007 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants