ghiscoding / Aurelia-Bootstrap-Plugins

Aurelia-Bootstrap-Plugins are Custom Elements to bridge with a set of 3rd party Bootstrap addons
https://ghiscoding.github.io/Aurelia-Bootstrap-Plugins
MIT License
45 stars 23 forks source link

Content of dropdown list not updated when bound collection is replaced #34

Open ghost opened 6 years ago

ghost commented 6 years ago

I use abp-select in a table like this:

<table>
  <tr repeat.for="item of items">
    <!-- some other columns -->
    <td>
      <abp-select
       collection.bind="item.subItems"
       selected-item.bind="item.selectedSubItem">
      </abp-select>
    </td>
  </tr>
</table>

Now the problem is that item.subItems may change (i.e. the entire array gets replaced); but when it does, the content of the dropdown list doesn't change.

By contrast, when I use a regular <select>, it works as expected, i.e. the <option> list get updated when item.subItems changes:

<table>
  <tr repeat.for="item of items">
    <!-- some other columns -->
    <td>
      <select
       value.bind="item.selectedSubItem">
        <option repeat.for="subItem of item.subItems" model.bind="subItem">
          ${subItem.option}
        </option>
      </select>
    </td>
  </tr>
</table>

Am I using <abp-select> incorrectly, or is this a bug?

ghiscoding commented 6 years ago

Right, I didn't think about that possibility and so the problem is that there's no collectionChanged in the Custom Element.

Possibly adding this piece of code would help

collectionChanged(newCollection, oldCollection) {
  this.collection = newCollection;
}

However this probably won't be enough, I'm not sure what will happen with your previous selections. It might be expected to reset the selection to empty? or remain the same selection, if it exist in the new collection?

Feedback and/or PR are welcome. Thanks

ghost commented 6 years ago

Oh ok I see. As for previous selections: That is a good question. For my specific use case, I don't care about that, as I explicitly reset it in my ViewModel anyway, so resetting would be fine. That doesn't have to be the case, though. For "least suprise", I'd try emulate Aurelia's behavior with <select> as closely as possible. I wrote a little Gist to test Aurelia's behavior - https://gist.run/?id=ca1f4b2b02e58d8aebfa58b78f27ea9e As you can see, the previous selection is preserved if it is found in the new or modified collection.

ghiscoding commented 6 years ago

Thanks for the emulation, but it might be missing a use case. I'm not sure what would happen if some (but not all) selections would not be part of the future collection. I'm assuming that user wouldn't want to see a selection that doesn't exist in the new collection. That might be tricky to deal with, or at least the steps would be to (1) keep a variable of the selections, (2) update the collection, (3) loop through the new collection with the previous selections and disregard the ones that don't exist.

I'm not sure how I would deal with number (3) yet.

Also please note that these plugins are all done in my spare time and it might take couple weeks before they are being addressed. However PR are very welcome and could potentially be addressed faster.

Thanks for using the plugin(s) and providing feedback :)

EDIT I tried to simply update the Gist you provided with a new collection. I added a new changeCollection() function so that it doesn't interfere with the reset() and inside it, I change the collection to be [1,3,4] (so I'm omitting 2) and if I select 2 and then click on the changeCollection(), it does keep the 2 as selected, which is probably not what the user expects. So it seems to keep the selected index, instead of the value, I don't think this is ideal though, I would prefer to do as I mentioned in my step (3)

ghost commented 6 years ago

Well, my take would be to keep it simple and work with indexes, just the way Aurelia seems to do with vanilla <select>. I actually think that's a sensible approach because it's fast. In those use cases where that's not good enough, custom programmatic logic for handling selections can be easily implemented in the view model, so you don't have to go out of your way here. Looping over the collection and doing actual comparison can be expensive for large arrays, and in many cases it's not needed - so users shouldn't be made to pay the price if they don't actually need the control to be that smart. Anyway, that's just my opinion, there are always different takes :-).

Won't have time for creating a PR any time soon though, sorry :-(. In the meantime, I'm using a more lightweight alternative - found a relatively simple way for directly bridging the bootstrap-select jQuery plugin with Aurelia via a custom attribute for <select> - at least works for my use case so far. Thanks for the nice work with this plugin, keep rocking!

ghiscoding commented 6 years ago

So I passed over half a day on this and it's halfway working. It works with a simple collection but as soon as the collection has things like subtext, content or others, it doesn't work for them.

I found out after a while that there was a timing issue and that placing the picker refresh inside a setTimeout does help but again I cannot fix the picker to work with subtext and others.

I pushed a new version 1.1.1 anyway, so if you use regular collection (string or object) and you aren't using thing like (subtext, content, ...) then you will be fine.

If anyone knows how to fix it, please help... here's my current code for the collection changed

  collectionChanged(newCollection, oldCollection) {
    setTimeout(() => {
      this.domElm
        .selectpicker('render')
        .selectpicker('refresh');
    });
  }
meash-nrel commented 6 years ago

i use content parameter to set the option as an image, so this is still not working for me in 1.1.1 and with the additional .render() and .refresh() calls on the element (set via element.bind).

ghiscoding commented 6 years ago

Unless you find an alternative, I have no clue how to deal with this problem since it's mainly a problem with the 3rd party plugin itself.

PR are welcome, else than that I already spent way to much time on this and out of ideas on how to fix it.

ghiscoding commented 6 years ago

Thanks to @Mobe91 A partial fix to collection was pushed on version 1.1.2. This is only partial, it works on simple collection (yay) but still doesn't work when we start adding subtext, style and other additions.

You can give it a try with a regular collection and see that it works. Give it a try on the demo on the first bootstrap-select which is a regular string collection.

If anyone find a way to fix the entire thing, that is changing a collection with subtext, style, ... Please make a PR and I'll be happy to review.