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

Select with multiple-selection for can.view.bindings (i.e. two-way binding via can-value) #551

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ghost

ghost commented Nov 15, 2013

This commit also adds tests that do pass with jQuery but fail every other library, because of a missing element.find()-function.
I’d appreciate any input on making this work cross-library!

0ff added some commits Nov 15, 2013

Implemented select with multiple-selection for can.view.bindings (i.e…
…. two-way binding via can-value)

Also added tests that do pass with jQuery but fail every other library, because of a missing element.find()-function. I’d appreciate any input on making this work cross-library!
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 20, 2013

Contributor

A short description / example of what's going on here would be really helpful.

Contributor

justinbmeyer commented Nov 20, 2013

A short description / example of what's going on here would be really helpful.

Show outdated Hide outdated view/bindings/bindings_test.js
equal(map.attr("color"), "green", "not yet updated from input");
can.trigger(inputs[0], "change")
equal(""+[map.attr("color.0"), map.attr("color.1")], ""+["red", "green"], "updated from input");

This comment has been minimized.

@justinbmeyer

justinbmeyer Nov 20, 2013

Contributor

could you use deepEqual here?

http://api.qunitjs.com/deepEqual/

@justinbmeyer

justinbmeyer Nov 20, 2013

Contributor

could you use deepEqual here?

http://api.qunitjs.com/deepEqual/

This comment has been minimized.

@justinbmeyer

justinbmeyer Nov 20, 2013

Contributor

Does this force the color attribute value into an array? But it works as a string value at first? I'm not sure about that ..

@justinbmeyer

justinbmeyer Nov 20, 2013

Contributor

Does this force the color attribute value into an array? But it works as a string value at first? I'm not sure about that ..

Updated the MultiSelect binding so that it should be compatible with …
…every lib that supports [element].val(value) (which is only jQuery right now).

Also rewrote “get” so that it now always returns a string (instead of an array) and combines multiple options via “;” (as jQuery does with .val())
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 6, 2013

Hey @justinbmeyer, these changes allow for the html-select element with multiple attribute.
Ex.:

<select can-value="color" multiple>
    <option value="red">Red</option>
    <option value="green">Green</option>
    <option value="ultraviolet">Ultraviolet</option>
</select>

I made changes as you suggested to always return a string. You were totally right, this actually caused me problems when using it as well. (Database Model was not expecting an array..).
I also tried to make the binding library-independent by using can.$ as the element selector (instead of this.element.find), but I could not find a solution for setting the value on the html-element without jQuery's .val().

The problem is not just iterating over all the options and setting them as selected - this will work but then [element][0].value will not correctly reflect the selected options. Setting [element][0].value seems to make some browsers behave very awkward (Safari then updates the selection, but only for single-selections. phantomJS seems to handle it differently). jQuery's .val(), on the other hand, sets the correct options selected and updates the .value correctly.

I'd love to get some input from you guys on this, as I believe this functionality (multi-select bindings) is a must-have for canjs.

ghost commented Dec 6, 2013

Hey @justinbmeyer, these changes allow for the html-select element with multiple attribute.
Ex.:

<select can-value="color" multiple>
    <option value="red">Red</option>
    <option value="green">Green</option>
    <option value="ultraviolet">Ultraviolet</option>
</select>

I made changes as you suggested to always return a string. You were totally right, this actually caused me problems when using it as well. (Database Model was not expecting an array..).
I also tried to make the binding library-independent by using can.$ as the element selector (instead of this.element.find), but I could not find a solution for setting the value on the html-element without jQuery's .val().

The problem is not just iterating over all the options and setting them as selected - this will work but then [element][0].value will not correctly reflect the selected options. Setting [element][0].value seems to make some browsers behave very awkward (Safari then updates the selection, but only for single-selections. phantomJS seems to handle it differently). jQuery's .val(), on the other hand, sets the correct options selected and updates the .value correctly.

I'd love to get some input from you guys on this, as I believe this functionality (multi-select bindings) is a must-have for canjs.

Fixed a bug in multiselect-bindings, that would cause clearing all se…
…lections to select “” with no option to unselect it.

@ghost ghost assigned justinbmeyer Jan 23, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 23, 2014

Contributor

I'm going to make this work with can.List by default and only use a delimitated string if the value of the cross bound property is a string.

Contributor

justinbmeyer commented Jan 23, 2014

I'm going to make this work with can.List by default and only use a delimitated string if the value of the cross bound property is a string.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Apr 1, 2014

Contributor

@justinbmeyer are you still planning on making changes to this? This is currently in minor as it is.

Contributor

matthewp commented Apr 1, 2014

@justinbmeyer are you still planning on making changes to this? This is currently in minor as it is.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Apr 1, 2014

Contributor

I think this is done in minor so this might need to be closed.

Contributor

justinbmeyer commented Apr 1, 2014

I think this is done in minor so this might need to be closed.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 3, 2014

Contributor

Yeah, I even documented it. Closing.

Contributor

justinbmeyer commented May 3, 2014

Yeah, I even documented it. Closing.

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