Skip to content
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

fixed issues with dynamic item collections in bootstrap-select #39

Merged
merged 3 commits into from
Mar 3, 2018

Conversation

Mobe91
Copy link
Contributor

@Mobe91 Mobe91 commented Feb 19, 2018

This PR fixes issues that I faced with dynamic collections and that are probably related to #34 .

Attached you find the example project that I used to work on this issue: aurelia-select-value-observer-test.zip.
With my changes applied, the initially selected country in the dropdown is correctly set to 'Italy'.

@ghiscoding
Copy link
Owner

Hello and thanks for the PR, I thought I would have time to review it today but I was busy releasing a new version of my other repo (Aurelia-Slickgrid). I hope to take a look at that in the coming days. Thanks again

@ghiscoding
Copy link
Owner

ghiscoding commented Feb 23, 2018

So I started testing this and here's a few observations

  1. the issue in Content of dropdown list not updated when bound collection is replaced #34 is partially fixed, the collection does get changed but
    • a. other properties like subtext, alternateText and others are still not working. Only regular array of string works correctly
    • b. also, if I try to preset with a new option from the collection, then it doesn't show up. However you said it works on your side, not sure why.
  2. the multiple selection is not working anymore, even though I have it set with multiple="true", I can only do a single selection at a time.
    • however preset of multiple options does work, something like this in my VM: this.picnicValue = [1, 3]; but after if I go in the UI and I want to start multiple, it only keeps last selection

The 1. is not so much of a concern since I still think the issue #34 is related to a problem with the 3rd lib itself.
However the 2. is very important and I canot merge your PR until that is fixed.

Not sure why the multiple selection are not working anymore, I hope you have an idea. (found it)

EDIT
I found the problem, you changed this line in the View:

- value.bind="value"
+ value.bind="selectedValue"

and since then the multiple selection is broken because of that simple change. It's a bit strange, because I don't actually use it (which is maybe the cause of some of #34 issues). If I change it back to the previous value, it works but then the collection change doesn't work anymore...

EDIT 2
If I completely remove the value.bind (again since my code doesn't really use it), then the multiple selections works again and if I put back the setTimeout that I had before, the collection change works again AND it fixes what didn't work for me in my observation at 1. b)

View

- value.bind="SelectedValue"

ViewModel

  collectionChangedObserver(newCollection, oldCollection) {
+    setTimeout(() => {
      this.domElm.selectpicker('refresh');
+    });
  }

With this edit, it might not be great to see the setTimeout but it's much more stable, so I think we should probably stick with that.
At the end of it, 1.a) still doesn't work (subtext and others) but it's much better than before.

Unless you have other comments, would you mind updating your code with the last changes I just wrote? Also my lint complains about 2 small things, var observer (var should be let) and finally in the renderSelection() function, the content of the second if has too much indentation (2 spaces too much).

@ghiscoding ghiscoding merged commit 3c25f8c into ghiscoding:master Mar 3, 2018
@ghiscoding
Copy link
Owner

ghiscoding commented Mar 3, 2018

Since I had no news from your side, I made the changes that I mentioned to fix multiple selection and collection delay issue. I will release a patch version 1.1.2

Thanks a lot for the contributions

@Mobe91
Copy link
Contributor Author

Mobe91 commented Mar 4, 2018

@ghiscoding Thanks for merging the changes and sorry for not answering. I am quite busy at the moment, maybe I will look at how to fix the remaining issues when I have more time and create another PR!

@Mobe91 Mobe91 deleted the issues/34 branch June 15, 2018 06:26
Mobe91 added a commit to ordami/Aurelia-Bootstrap-Plugins that referenced this pull request Aug 27, 2018
…oding#39)

* fixed issues with dynamic item collections in bootstrap-select

* remove unnecessary`value.bind`

* fix multiple selection and add refresh delay to fix collection change
Mobe91 pushed a commit to ordami/Aurelia-Bootstrap-Plugins that referenced this pull request Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants