davidstutz / bootstrap-multiselect

JQuery multiselect plugin based on Twitter Bootstrap.
https://davidstutz.github.io/bootstrap-multiselect/
Other
3.67k stars 1.98k forks source link

Knockout async binding issues #214

Closed thj-dk closed 10 years ago

thj-dk commented 10 years ago

I'm having some problems with the Knockout binding. I have the following markup:

<select id="input-client" class="form-control" data-bind="
    options: clients,
    optionsText: 'name',
    optionsValue: 'id',
    value: selectedClientId,
    multiselect: {}">
</select>

This correctly renders the multiselect dropdown, but if the data is loaded asynchronously, some times the dropdown isn't rendered. I suspect that it has something to do with the multiselect list not being rebuild after having added objects to the options observable.

Does anyone know how to solve this issue?

BrewDawg commented 10 years ago

I have this code:

<select id="mySelect" class="multiselect" multiple="multiple" data-bind="
    options: clientsArray,
    optionsText: 'name',
    optionsValue: 'id',
    selectedOptions: clientIDsPopulated,
    multiselect: true">
</select>

and then I do this:

ko.applyBindings(vm, $('#container')[0]);

Which correctly turns it into a Bootstrap-Multiselect control

However, my clientsArray (which is used for my options) is empty at the time I call ko,applyBindings(). Then later I load the data and it never shows. my clientsArray is a ko.observableArray() and everything works if I populate it before I call ko.applyBindings. It looks like you cannot load data after calling ko.applyBindings() when using a bootstrap-multiselect. Too bad, this will probably be a show stopper for us. Maybe the authors will come through with some information we don't know ...

thj-dk commented 10 years ago

Exactly. That was what I was hoping for :-)

BrewDawg commented 10 years ago

Ha, I found out how to make it work (but I'd call it a work around)

this.populate = function () {
    self.clientsArray(self.oneThousandClients());
    $("#mySelect").multiselect('rebuild');
};

Now, Ideally they would make sure their ko binding logic subscribes to the change event of the ko.observableArray() and simply call 'rebuild' but this works for me. I call populate() on the button click in my test code long after ko.applyBindings() and it reloads with the right data.

thj-dk commented 10 years ago

I was hoping for a more permanent solution, and a fix to the binding extension :-)

BrewDawg commented 10 years ago

I've been looking into it, they have a binding handler right at the top of the JavaScript file:

 ko.bindingHandlers.multiselect = {
     // logic in here
}

I've already figured out how to determine if the selectOptions is a ko.observableArray() and now I'm subscribing to the change event. My intention is to call multiselect('rebuild') when it changes so this will all be automatic. If I can get this to work I'll issue a pull request and get this into the control.

BrewDawg commented 10 years ago

Okay, I have the fix, maybe the 'multiselect' team will implement this?

if (typeof ko !== 'undefined' && ko.bindingHandlers && !ko.bindingHandlers.multiselect) {
        ko.bindingHandlers.multiselect = {
            init: function(...) {},
            update: function(...) {

               var config = ko.utils.unwrapObservable(valueAccessor());
               var selectOptions = allBindingsAccessor().options;
               var ms = $(element).data('multiselect');

               // BEGIN FIX
               if (isObservableArray(selectOptions)) {
                   selectOptions.subscribe(function (theArray) {
                       $(element).multiselect('rebuild');  // <== BINGO !!
                   });
               }
              // END FIX

               if (!ms) {
                  $(element).multiselect(config);
               }
               else {
                  ms.updateOriginalOptions();
                  if (selectOptions && selectOptions().length !== ms.originalOptions.length) {
                     $(element).multiselect('rebuild');
                  }
               }
            }
        };
    }

    // ADDED FUNCTION BECAUSE knockout DOESN'T HAVE THIS
    function isObservableArray(obj) {
        return ko.isObservable(obj) && !(obj.destroyAll === undefined);
    };

Now when I load my multiselect select list after ko.applyChanges it still updates without any effort on my part

BrewDawg commented 10 years ago

I submitted a pull request for the fix, hopefully the powers that be will review the changes and accept the request, it works great, you can see the pull request here:

https://github.com/davidstutz/bootstrap-multiselect/pull/219

If you want to test it pull it from my repo which will be removed once the pull request is accepted/rejected. You can go into raw mode and copy it down ...

https://github.com/BrewDawg/bootstrap-multiselect/blob/master/js/bootstrap-multiselect.js

thj-dk commented 10 years ago

So far so good, Brew. I have pulled in your request and tested it a bit, and so far it seems to be working perfect. I'll test it some more in the coming days. Thanks for taking your time :-)

sgentile commented 10 years ago

could you give an example of how you are using this - I just took this branch and it's not working for me it always selects all for me and it's not updating the observableArray that tracks selectedOptions

BrewDawg commented 10 years ago

You are correct, it doesn't update when selectedOptions changes but we have fixed that as well, we have this thing bullet proof. There was really some work that needed to be done to make this control truly support Knockout. All it was able to do was load data and only if the options was populated before ko.ApplyBindings. I will post a patch tonight that fully supports all aspects of Knockout, we are using it heavily already and without fail, including selectedOptions.

sgentile commented 10 years ago

ok, looking forward to seeing that - thank you!

BrewDawg commented 10 years ago

By the way, if you want the a very cool way to work with knockout (which is totally free) checkout http://www.tiraggo.com/tiraggojs/ and it's all free, it's generated from your database, I've been building sites using (as have others) it's awesome. I wrote it to just to be up-front about it ...

davidstutz commented 10 years ago

Ok, to be truly honest, I have not worked with Knockout at all. Until now the Knockout integration was always contributed by some users. So I would appreciate a completely bullet proof Kncokcout integration, too!

BrewDawg commented 10 years ago

You got it, as soon as I get home I'll issue a pull request with full support for Knockout. I'll put a demo page up on my site too showing it working fully in all cases.

BrewDawg commented 10 years ago

Okay, here's basically full support for knockout (it requires KO 3.0 because I used the new ko.observableArray smart updating features)

https://github.com/BrewDawg/bootstrap-multiselect/blob/master/js/bootstrap-multiselect.js

You can click "Raw" and copy the source down over your current bootstrap-multiselect.js

sgentile and tommyj can give this a test, if they say it's good (and it should be) I will issue a pull request. Now you can change the selectedOptions: at any time and the multiselect will reflect the changes.

// The first 6 will be selected assuming your id's 
// are numbers but they can be strings too)
this.mySelectedIds([1,2,3,4,5,6]); 

Here is what I did:

Major overhaul of knockout support, added clearSelection() method and dramatically improved performance of getOptionByValue() and getInputByValue()

Finally, there is a CSS bug that will allow the combobox to grow larger than it's boostrap column and intrude into other columns, I have the CSS fix for that too, I'm very surprised nobody found that as of yet. Too see this bug just add some very long names and you'll see it when you have like 3 selected or so. I will post the CSS fix as well but not tonight, getting late. Basically, you need to use ellipses and give the text a width.

sgentile commented 10 years ago

I'll give it a whirl - I appreciate you putting in this work to make it happen - thanks!

sgentile commented 10 years ago

Do you mind sharing - maybe jsfiddle or something - your test scenario you are using ?

ie. this is an example I have:

I'm not getting any updates to the selectedOptions ?

I'm not sure if I'm missing something

var selectedChoice = ko.observable();
    var availableSet = [{ setName: "Valentines" }, { setName: "Christmas" }, { setName: "Father's Day" }, { setName: "Mother's Day" }, { setName: "Back to School" }, { setName: "Superbowl" }];
    var multiSelectInitOptions = {
        includeSelectAllOption: true
    };

    var selectedOptions = ko.observableArray([availableSet[0], availableSet[1]]);
<select multiple="multiple" data-bind="options: availableSet, optionsText: 'setName', selectedOptions: selectedOptions,  multiselect: multiSelectInitOptions"></select>

<p>Selected Options:</p>
                <ul data-bind="foreach: selectedOptions">
                    <li data-bind="text:setName"></li>
                </ul>

(by the way, I'm using the bootstrap 2.3 on my project)

(Side note, I'm going to help get this AMD ready as well - I'm using Durandal/RequireJS)

BrewDawg commented 10 years ago

Sure, no problem, I took a look at your code, I've never seen anyone use a ko.observableArray like that, not sure that's even valid, take a look at this, works great ...

http://jsfiddle.net/TQ7eY/11/

thj-dk commented 10 years ago

That looks perfect Brew. Thank you very much. I haven't got the time today to fully test it, but i will do in one of the coming days. Good work!

sgentile commented 10 years ago

So I was able to duplicate your code, and with that CSS it is displaying right. However, my "mySelectedOptions" is not getting written to - the subscribe isn't getting called ?

sgentile commented 10 years ago

I am using jQuery 1.9.1 if that matters.

BrewDawg commented 10 years ago

Are you using knockout 3.0 ? It definitely works.

Here's a new JSFiddle using JQuery 1.9.1

http://jsfiddle.net/TQ7eY/13/

You can see it in the "External Resources" on the left if you hover over jquery.min.js

The best thing you can do is build a very simple html page out of the jsFiddle above, would take 5 to 10 minutes at best, my guess is it will work.

You should be able to change my JSFiddle and do an update if you can show me something that doesn't work, my guess is it's something in your code ...

sgentile commented 10 years ago

I've narrowed down the problem - and I know it works in your scenario. however, for whatever reason, whether it's a timing issue, etc... the following code doesn't trigger the update on my scenario:

this.$select.change();
this.options.onChange($option, checked);

I know the wiring is right, as if it updated the selectedOptions - ie. I add a '1' to it, while in console, I can see the checkbox get updated.

I'll see if I can figure out why - at first I wondered if it had to do with the ordering of the handlers, but I had thought that was fixed in 3.0.0. The way it is signaling a change via jquery vs. triggering a change by directly changing the observable can be tricky. I'd like to create a reproducible scenario for you to see - meanwhile I'll see if I can track down why this isn't picking up the jquery change call.

BrewDawg commented 10 years ago

Interesting, here's something that might help. The only thing unique about the multiselect-bootstrap control when it comes to Knockout is when you use "multiselect:" in the data-bind statement. Bootstrap goes through all of the items in the data-bind statements and finds binding handlers for them all. For instance:

<select data-bind="
    options: myOptions,
    optionsText: 'name',
    optionsValue: 'id',
    selectedOptions: mySelectedOptions">
</select>

All of those bindings above are for a normal combobox and knockout itself handles those. Wiith the multiselect-boostrap control we add multiselect: like this

<select data-bind="
    options: myOptions,
    optionsText: 'name',
    optionsValue: 'id',
    selectedOptions: mySelectedOptions,
    multiselect: true"> <!-- HERE -->
</select>

And that is what makes it call the multiselect-bootstrap ko handler

 ko.bindingHandlers.multiselect = { } // method

So, all that is to say that if you remove the things I've removed in my original sample (see this new JsFiddle) http://jsfiddle.net/TQ7eY/15/ you'll see everything still works, I didn't change any javascript code at all, just the markup, my comments are in the markup, try the same with your app and see if everything works when your control is acting as a regular ol' combobox.

Meantime, I'll research

this.$select.change();
this.options.onChange($option, checked);

I'd really like to see you create a JsFiddle, I really want to issue a pull request and get this into the control permanently but we need to make sure there is no issue here. Let me know what you find out, again, if you can post a jsfiddle sample or even email an html page (or zip) demonstrating the problem to mike.griffin@brewdawg,com I'll debug it.

BrewDawg commented 10 years ago

Any news, I'm going to issues a pull request otherwise, we are using the exact version of code I have modified in a very large scale application without any errors or issues whatsoever ...

sgentile commented 10 years ago

I'd go ahead with it - I only had a window of opportunity to prototype using this and I've had to move on - I don't know if the issue had to do with something around durandaljs lifecycle of binding. Thanks for helping work through it

BrewDawg commented 10 years ago

Cool, I'll issue a pull request this weekend. Like I said, we're using this heavily in an large app everywhere without issues and with full knockout binding everywhere ....

BrewDawg commented 10 years ago

Okay, folks, the "Full Knockout Support" pull request has been issued, can't wait till' it's added.

AJPratt commented 10 years ago

Nice work BrewDawg. I found a few issues however, the generated ul list is not initiated correctly. It displays properly if the observable collection the select list is bound to is changed or modified but there is nothing in the initial list to start. The hidden select behind the scenes has the initial values but not the ul until the ko array is modified. I have a modification that will initialize properly. It works if you have the list bound to a collection of objects and have a "optionsValue" and "optionsText" parameter specified but I don't have it working for a simple observable array yet. Also I noticed that the selectedOptions property can still contain a selected value of an item even if that item is no longer in the list. The selectedOptions property seems to not get updated if the entire options collection is replaced until one of the new items is selected. I'm working on that one too.

AJPratt commented 10 years ago

I added / modified the following in the custom binding...

            //Added custom to initialize for KO properly
            var options = ko.toJS(allBindingsAccessor().options);
            var optionsValue = allBindingsAccessor().optionsValue;
            var optionsText = allBindingsAccessor().optionsText;

            $(element).multiselect(config, null, { options: options, optionsValue: optionsValue, optionsText: optionsText });

then in the function call I modified to pass in the initOptions...

$.fn.multiselect = function (option, parameter, initOptions) {
    return this.each(function() {
        var data = $(this).data('multiselect');
        var options = typeof option === 'object' && option;

        // Initialize the multiselect.
        if (!data) {
            $(this).data('multiselect', (data = new Multiselect(this, options, initOptions)));
        }

        // Call multiselect method.
        if (typeof option === 'string') {
            data[option](parameter);
        }
    });
};

then in the constructor I use the initOptions to build the options...

    this.buildDropdownOptions(initOptions);

I modified the buildDropdownOptions by adding the following...

    buildDropdownOptions: function (initOptions) {         
        if (this.$select.children().length == 0 && (initOptions)) {
            var parent = this;
            $.each(initOptions.options, function (i, option) {
                parent.createOptionValuePre(eval('option.' + initOptions.optionsValue), eval('option.' + initOptions.optionsText));
            });
        }

        ...

            // Other illegal tags will be ignored.
        }, this));

Which you'll see uses the following new method (createOptionValuePre)...

    createOptionValuePre: function(value, label) {
        var inputType = this.options.multiple ? "checkbox" : "radio";

        var $li = $(this.templates.li);
        $('label', $li).addClass(inputType);
        $('label', $li).append('<input type="' + inputType + '" />');

        //var selected = $(element).prop('selected') || false;
        var $checkbox = $('input', $li);
        $checkbox.val(value);

        if (value === this.options.selectAllValue) {
            $checkbox.parent().parent().addClass('multiselect-all');
        }

        $('label', $li).append(" " + label);

        this.$ul.append($li);
    },
AJPratt commented 10 years ago

So then to use it it looks like this...

            <select multiple="multiple" 
                    data-bind="options: myObservableCollection, 
                               optionsValue: 'tId', 
                               optionsText: 'Name', 
                               selectedOptions: mySelections, 
                               multiselect: {
                                                buttonClass: 'btn btn-default btn-grey',
                                                numberDisplayed: 2
                                             }">
            </select>
josuaobando commented 10 years ago

Hello I am using this component for an Angularjs application. So, I need to configure multi-languages ​​(en, es, pt) with filters Angularjs ("'KEY' | translate"). For example: selectAllText, filterPlaceholder, nonSelectedText, nSelectedText.

Best regards

ghiotion commented 9 years ago

There was a reference a while back about getting this AMD ready for use in require.js. I'd REALLY like to do that in a project I'm working on so that the multiselect binding is available. Can anyone offer any guidance? Something like...

        'bootstrap-multiselect': {
            deps: ['jquery', 'bootstrap'],
            exports: 'ko.bindingHandlers.multiselect'
        }