rniemeyer / knockout-jqAutocomplete

knockout-jqAutocomplete is a Knockout.js plugin designed to work with jQuery UI's autocomplete widget.
MIT License
49 stars 20 forks source link

inputProp is supported on binding update #7

Closed eugenet8k closed 9 years ago

eugenet8k commented 9 years ago

The missing functionality of taking a property value of an updated value object was added.

I stuck on the problem that even if I set options.inputProp to some property name, on updating of the binding the whole instance of updated data object was set to HTML Input element's value. Because of this code

    this.update = function (element, valueAccessor) {
        var options = unwrap(valueAccessor()),
            value = unwrap(options && options.value);

        element.value = value;
    };

I updated the logic and now we try to get a property value instead the whole data object value if options.inputProp is set

    this.update = function (element, valueAccessor) {
        var options = unwrap(valueAccessor()),
            value = unwrap(options && options.value),
            props = self.getPropertyNames(valueAccessor);

        element.value = value && props.input ? value[props.input] : value;
    };

Please let me know if it has any problems or I missed something important. Thanks for the great component!

rniemeyer commented 9 years ago

Thanks! I'll take a look.

eugenet8k commented 9 years ago

@rniemeyer thanks for a quick feedback. I am not sure that I've seen any suitable example from what you provided with the component to reproduce the problem.

My best guess here is to simulate a case where some model already has a value in an observable so the initialization of the autocomplete input that is binded to that observable will invoke binding update method where the new code will perform.

I updated your examples jsfiddle with the mentioned use-case, please take a look http://jsfiddle.net/ievgent/uGGb8/7/

If I set a random object value { id: 2, name: "item 2", myLabel: "item label 2"} to this.objectValue = ko.observable() the input element will show "[object Object]" which is the bug I am trying to solve here.

eugenet8k commented 9 years ago

Well after thinking for a bit I must to admit that my specific case where I am having problem is that I store in the observable the whole data object (item from autocomplete items), where in your examples you mostly save a specific value property of the item (i.e. "id" property). So the code element.value = value; works for those examples, because value is a scalar. But in my case it doesn't work, 'cause my value is an object (item).

I use the following settings for jqAuto: jqAuto: { value: hostData, source: getAutocompleteHosts, inputProp: 'Name', template: 'autocompleteEntityItemTemplate' }

I don't set valueProp because I set the whole item back to model and I don't set labelProp because I use a template.

Therefore in the cases where both inputProp and valueProp are set, my code update will bring a problem. Because it will try to get a property from a scalar value. So I think it need to be adjusted to something like:

this.update = function (element, valueAccessor) {
    var options = unwrap(valueAccessor()),
        value = unwrap(options && options.value),
        props = self.getPropertyNames(valueAccessor);

    element.value = value && $.isPlainObject(value) && props.input && !props.value 
            ? value[props.input] : value;
};

Sorry for bothering you! Please take a look and share your thoughts...

eugenet8k commented 9 years ago

Oh man, after I looked deeper I figured that I am probably all wrong, because you supposed to use dataValue settings property to pass the whole item instance to a model. After I've updated my example with this

jqAuto: { value: hostName, dataValue: hostData, source: getAutocompleteHosts, inputProp: 'Name', template: 'autocompleteEntityItemTemplate' }

it's working great! I had just to make a foo property hostName to carry those string values.

Well, I am deeply sorry for taking your time and not carefully learning manuals and checking the source code. We should close this issue as designed and reject the pull request.

rniemeyer commented 9 years ago

@ievgen no problem! I hadn't had a chance to look into it.