SelectView should work properly with optionValuePath and selection #9468
Conversation
chrmod
changed the title
chrmod
force-pushed
the
select-value-path-fixes
branch
from
9fe37fc
to
6921da2
Compare
|
Would like to hear some opinions on this one. Especially if support for multiple selection should be a part of this PR or another one. |
|
@chrmod Unfortunately, I've got a bit of bad news for you. Right now we're planning to deprecate Ember.Select and point people towards a new (yet to be added) component version. I'm not sure that we want to merge any changes to |
|
Don't get me wrong, but will not cry after Ember.Select. It is a piece of the framework that gives many people a lot of problems. This PR was created by necessity, as it fixes critical use cases in apps we build. Looking forward for info of your official discussion. |
|
I'm all for the bright blue sky future but fixing what is there currently should take priority over an as yet unknown future promised land. If there is no viable alternative right this minute then this fix should be merged asap. For many users the simple alternative is to use another framework where something as basic as a select box actually works. I want select to not be so broken today and when its replacement lands then great, we can all migrate to that. Please dont hold out on this PR its not taking the framework in an undsirable direction, or increasing api surface area, its just repairing what's already there. IMO Ember needs to provide solutions today, and whilst planning for the future is a good thing, if Ember can't do the basics now who cares about all the advanced stuff or future advanced stuff. |
|
See also #5259 |
|
Hi @ahacking I'd like to chat with you sometime if you're available. Please contact me at im.mmun@gmail.com or mmun on #emberjs. |
| return get(this, labelPath); | ||
| }).property(labelPath)); | ||
| var labelPath = getWithDefault(this, 'parentView.optionLabelPath', 'content'); | ||
| defineProperty(this, 'label', computed.alias(labelPath)); |
There was a problem hiding this comment.
Can you import alias directly here (instead of pulling it off of computed)?
chrmod
force-pushed
the
select-value-path-fixes
branch
from
6921da2
to
4787716
Compare
|
@rwjblue code is now updated to current master and your suggestion are applied. Please let me know what you think. |
| } | ||
| }); | ||
| }.bind(this)); |
There was a problem hiding this comment.
Function.prototype.bind is not available in ES3 browsers (which we must support). Please refactor to use a closure scoped variable (like var viewContext = this).
There was a problem hiding this comment.
This is what caused the Travis build to fail.
chrmod
force-pushed
the
select-value-path-fixes
branch
from
4787716
to
30ee289
Compare
| collection: Ember.A([{name: 'Wes', value: 'w'}, {name: 'Gordon', value: 'g'}]), | ||
| val: 'g', | ||
| selectView: SelectView, | ||
| template: Ember.Handlebars.compile(templateString) |
There was a problem hiding this comment.
Can you change this to just compile (instead of Ember.Handlebars.compile -- which is a compat mode alias for the HTMLBars compiler).
chrmod
force-pushed
the
select-value-path-fixes
branch
from
30ee289
to
e5665ef
Compare
SelectView should work properly with optionValuePath and selection
|
@chrmod - Thank you! |
SelectView was not working property when
optionValuePathandselectionproperties were set.selectionandvalueproperties did not update, alsoselectedattribute was not set on option tag.Typical use case in which SelectView content was bound to ember-data records array and selection was bound to some ember-data object fail to set Select into proper state. Example: http://emberjs.jsbin.com/zemomorunu/1/
This PR aims to improve SelectView so its content can be bound to array of any Ember objects.
TODO:
multiple=true