Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

I changed the way how optionsValue is executed. #154

Merged
merged 3 commits into from

4 participants

@martinemmert

I think optionsValue and optionsText should share the same functionality, otherwise its very confusing.
I got stuck for several hours on my current project because of optionsValue is not executing function expressions like optionsText.

Hope it helps.

Best Regards

Martin

@SteveSanderson

Thanks for this. If you are able to complete the pull request then I will be able to merge it in. What's needed is:

  • Ensure the code is finalised (remove the commented-out code, and update the indentation to match the rest of the file)
  • Add test cases to prove the new functionality

Sorry for having to ask for that, but as an OSS project, it needs to stay healthy by getting the same coding style and level of test coverage from all contributers :)

Thanks for your contribution.

@evoskuil

I incorporated this same change to my local version as well; however I found it insufficient for what I needed.

If you provide a string property name for optionsValue the existing implementation will map the value to the option element value attributes - and - it will round trip the selected property value(s) via selectedOptions. If you provide no optionsValue it will not set the option value attributes (which is understandable, since a complex object is being mapped) - and - it will round trip the complex object. However, if one simply mimics the behavior of optionsText by providing a selector function the result of the selection will be mapped to the option value attributes but will also be round-tripped via selectedOptions. In other words there's no way to select a value to be mapped into the options value attribute while still round-tripping the complex object.

That makes the binding overly cumbersome when you want to bind something to the select element (such as Eric Hynds' jQuery UI MultiSelect Widget). It's absolutely necessary that knockout map values to the options, but the only way to do this, even with the proposed enhancement, is to round-trip the simple value array via selectedOptions. This then, if dealt with external to knockout source, requires a computed observable to map the simple array into the object array you are probably using (and can use if you don't provide any optionsValue).

So I locally made two additional changes, one within the selectedOptions binding init and the other in update - about 16 lines of source total. These changes detect the presence of an optionsValue mapping function and if present use the same function to round trip the options objects as the selectedOptions values.

Ideally the existing string-based value selector would also round trip the complex object - since it has the ability to do so. But it just writes the array of values extracted from the options elements. However, by providing function-based selection, its now possible to select the same value and instead round trip the complex objects without breaking existing implementations.

I'd be happy to incorporate all three changes if people are interested.

@mbest
Owner

@evoskuil - I don't really understand all that you're saying, especially what you mean by "round trip". I'm thinking that including an implementation of #382 will allow you to set the value any way you want. So I was thinking of closing this issue. But it seems you thought selectedOptions needed to change also. Awaiting your reply...

@evoskuil

It's probably easiest for me to explain by showing the necessary mode to selectedOptions:

'init': function (element, valueAccessor, allBindingsAccessor) {
    ko.utils.registerEventHandler(element, "change", function () { 

        // HACK: honor optionsValue lambda for value mapping
        // +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
        var optionsValueValue = allBindingsAccessor()['optionsValue'];
        var selectedValues = ko.bindingHandlers['selectedOptions'].getSelectedValuesFromSelectNode(this);
        if (typeof optionsValueValue == "function") {
            var optionsValue = ko.utils.unwrapObservable(allBindingsAccessor()['options']);
            selectedValues = ko.utils.arrayFilter(optionsValue, function (option) {
                return ko.utils.arrayFirst(selectedValues, function (selection) {
                    return optionsValueValue(option) === selection;
                });
            });
        }
        // +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

        var value = valueAccessor();
        if (ko.isWriteableObservable(value))
            value(selectedValues);
        else {
            var allBindings = allBindingsAccessor();
            if (allBindings['_ko_property_writers'] && allBindings['_ko_property_writers']['value'])
                allBindings['_ko_property_writers']['value'](selectedValues);
        }
    });     
},
@mbest
Owner

Thanks! That does help. Basically, you want the array filled by selectedOptions to use the objects in the options array even though you've set a value for the options. Did you also have to change the update function for selectedOptions?

@mbest
Owner

@evoskuil - I think #382 would solve the problem you had without having the change selectedOptions. That's because you could leave off optionsValue, which would cause options to set the object as a value, but then you could set a value attribute using the attr/value binding.

Because of the difference in how optionsValue and attr/value bindings work, I'm going to leave this pull request open and mark it for inclusion in v2.2.

@evoskuil

Yes, forgot to include the update function change:

    'update': function (element, valueAccessor, allBindingsAccessor) {
        if (element.tagName != "SELECT")
            throw new Error("values binding applies only to SELECT elements");

        var newValue = ko.utils.unwrapObservable(valueAccessor());
        if (newValue && typeof newValue.length == "number") {
            var nodes = element.childNodes;
            for (var i = 0, j = nodes.length; i < j; i++) {
                var node = nodes[i];
                if (node.tagName == "OPTION") {

                    // HACK: honor optionsValue lambda for value mapping
                    // +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
                    var nodeValue = ko.selectExtensions.readValue(node);
                    var optionsValueValue = allBindingsAccessor()['optionsValue'];
                    var matched = typeof optionsValueValue == "function" ?
                        ko.utils.arrayFirst(newValue, function (newValueElement) { return optionsValueValue(newValueElement) === nodeValue; }) != null :
                        ko.utils.arrayIndexOf(newValue, nodeValue) >= 0;
                    ko.utils.setOptionNodeSelectionState(node, matched);
                    // +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
                }
            }
        }
    }
@evoskuil

What's interesting is that this is the behavior you get when you don't specify optionsValue (using the current implementation). The selected object is set as the value. Once you use optionsValue to select an object (i.e. by matching the value of property by name), the value that is set into selectedOptions is no longer the object but the property value. I didn't attempt to change that behavior, because it would cause compatability problems and because I personally wouldn't ever use it, but I don't really like the way it works. Thanks for looking this over.

@evoskuil

FYI, while integrating my hack with ko 2.1 I realized the above documentation missed a couple of lines of integration (listed for 2.0.0):

            var value = valueAccessor();
            if (ko.isWriteableObservable(value))
                value(selectedValues);
            else {
                var allBindings = allBindingsAccessor();
                if (allBindings['_ko_property_writers'] && allBindings['_ko_property_writers']['value'])
                    allBindings['_ko_property_writers']['value'](selectedValues);
            }
        });     
    },
    'update': function (element, valueAccessor, allBindingsAccessor) {
@SteveSanderson

Is this issue left open just to provide further context for #382, or is the original pull request still applicable? Michael, could you clarify? Thanks!

@mbest
Owner

I think this pull request is valuable even if #382 gets added. That's because you'd have to use the value binding to substitute for optionsValue, but value has extra overhead of setting up event handlers.

@SteveSanderson

OK, so all we'd do for #154 is evaluate optionsValue if it happens to be a function, as in the original pull request? There's no need to modify selectedOptions or anything else?

That would make sense as it's just a generalisation of optionsValue interpreting a string as a property name.

@mbest mbest was assigned
@mbest
Owner

OK, so all we'd do for #154 is evaluate optionsValue if it happens to be a function...

Yes.

There's no need to modify selectedOptions or anything else?

His use case was to have selectedOptions use the original objects even though he was also using optionsValue to set a text value for the options. This will be supported via #382 using the attr/value binding instead of optionsValue.

I've updated the pull request and added a spec. Let me know what you think.

@SteveSanderson SteveSanderson merged commit de95292 into master
@SteveSanderson

This looks perfect. I really like the simplified implementation. Thanks!

@mbest
Owner

You're welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 13, 2012
  1. @martinemmert @mbest

    Changed the way the binding 'optionsValue' is executed.

    martinemmert authored mbest committed
  2. @mbest

    #154 - make optionsValue and optionsText consistent using a helper fu…

    mbest authored
    …nction
    
    also clean up some duplicate code
  3. @mbest
This page is out of date. Refresh to see the latest.
Showing with 27 additions and 15 deletions.
  1. +14 −1 spec/defaultBindingsBehaviors.js
  2. +13 −14 src/binding/defaultBindings.js
View
15 spec/defaultBindingsBehaviors.js
@@ -476,12 +476,25 @@ describe('Binding: Options', {
{ name: 'bob', job: 'manager' },
{ name: 'frank', job: 'coder & tester' }
]);
- testNode.innerHTML = "<select data-bind='options:myValues, optionsText: function (v) { return v[\"name\"] + \" (\" + v[\"job\"] + \")\"; }, optionsValue: \"id\"'><option>should be deleted</option></select>";
+ testNode.innerHTML = "<select data-bind='options:myValues, optionsText: function (v) { return v[\"name\"] + \" (\" + v[\"job\"] + \")\"; }'><option>should be deleted</option></select>";
ko.applyBindings({ myValues: modelValues }, testNode);
var displayedText = ko.utils.arrayMap(testNode.childNodes[0].childNodes, function (node) { return node.innerText || node.textContent; });
value_of(displayedText).should_be(["bob (manager)", "frank (coder & tester)"]);
},
+ 'Should accept a function in optionsValue param to select subproperties of the model values (and use that for the option text)': function() {
+ var modelValues = new ko.observableArray([
+ { name: 'bob', job: 'manager' },
+ { name: 'frank', job: 'coder & tester' }
+ ]);
+ testNode.innerHTML = "<select data-bind='options: myValues, optionsValue: function (v) { return v.name + \" (\" + v.job + \")\"; }'><option>should be deleted</option></select>";
+ ko.applyBindings({ myValues: modelValues }, testNode);
+ var values = ko.utils.arrayMap(testNode.childNodes[0].childNodes, function (node) { return node.value; });
+ value_of(values).should_be(["bob (manager)", "frank (coder & tester)"]);
+ var displayedText = ko.utils.arrayMap(testNode.childNodes[0].childNodes, function (node) { return node.innerText || node.textContent; });
+ value_of(displayedText).should_be(["bob (manager)", "frank (coder & tester)"]);
+ },
+
'Should update the SELECT node\'s options if the model changes': function () {
var observable = new ko.observableArray(["A", "B", "C"]);
testNode.innerHTML = "<select data-bind='options:myValues'><option>should be deleted</option></select>";
View
27 src/binding/defaultBindings.js
@@ -227,23 +227,22 @@ ko.bindingHandlers['options'] = {
for (var i = 0, j = value.length; i < j; i++) {
var option = document.createElement("option");
+ function applyToObject(object, predicate, defaultValue) {
+ var predicateType = typeof predicate;
+ if (predicateType == "function") // Given a function; run it against the data value
+ return predicate(object);
+ else if (predicateType == "string") // Given a string; treat it as a property name on the data value
+ return object[predicate];
+ else // Given no optionsText arg; use the data value itself
+ return defaultValue;
+ }
+
// Apply a value to the option element
- var optionValue = typeof allBindings['optionsValue'] == "string" ? value[i][allBindings['optionsValue']] : value[i];
- optionValue = ko.utils.unwrapObservable(optionValue);
- ko.selectExtensions.writeValue(option, optionValue);
+ var optionValue = applyToObject(value[i], allBindings['optionsValue'], value[i]);
+ ko.selectExtensions.writeValue(option, ko.utils.unwrapObservable(optionValue));
// Apply some text to the option element
- var optionsTextValue = allBindings['optionsText'];
- var optionText;
- if (typeof optionsTextValue == "function")
- optionText = optionsTextValue(value[i]); // Given a function; run it against the data value
- else if (typeof optionsTextValue == "string")
- optionText = value[i][optionsTextValue]; // Given a string; treat it as a property name on the data value
- else
- optionText = optionValue; // Given no optionsText arg; use the data value itself
- if ((optionText === null) || (optionText === undefined))
- optionText = "";
-
+ var optionText = applyToObject(value[i], allBindings['optionsText'], optionValue);
ko.utils.setTextContent(option, optionText);
element.appendChild(option);
Something went wrong with that request. Please try again.