skratchdot / react-bootstrap-multiselect

A multiselect component for react (with bootstrap). This is a react port of bootstrap-multiselect.
http://projects.skratchdot.com/react-bootstrap-multiselect/
Other
119 stars 62 forks source link

`multiple` option generates 2 `ul`s in the dom #78

Open South-Paw opened 7 years ago

South-Paw commented 7 years ago

The Problem

I found today that when I use the multiple option, the bootstrap-multiselect generates two uls for the dropdown of options, with the second being populated by the selectable options.

image

It would seem that with Bootstrap v3 this is not an issue as both of these uls are shown at the same time (the empty one just appears as 2px of borders and is unnoticeable).

However if you are using Bootstrap v4, the new dropdown they have implemented only applies styles to the first child ul instead of both - meaning that the second ul containing the options is not shown/visible.

image

The little border there is the empty dropdown list.

Some Details

We use a modified source of this plugin to work around the bootstrap/jQuery requirement (I think this is mentioned in #73 if it matters) so I started off by checking that it is not our changes causing the issue.

It didn't appear to be in our changes so I checked the demo page for this plugin and you can see the issue by inspecting the dom on the multiple select dropdowns (example attached).

image

I then went and checked that this issue is not from the parent bootstrap-multiselect plugin and can confirm that their multiple examples on the demo page do not exhibit this problem.

Fixing it

In the in index.js and in the componentDidMount() function; there is a call on line 80 to $this.$multiselect.multiselect($this.getOptionsFromProps()); directly followed by a second call to $this.setOptionsFromProps();.

It would seem that the second call there is then building another dropdown list with no items in it.

If we comment out line 81 (the $this.setOptionsFromProps();) then the extra ul disappears - the plugin also seems to continue to work fine without this line.

Questions

  1. Is there any harm in commenting out this line? We're unsure if it affects/breaks anything else we are unaware of.
  2. Is this a change that should be added to this repo?

Thanks for you time, keep up the awesome work! 👍

ttessarolo commented 6 years ago

Same issue. I've followed your suggestion and now it works. I'm using React@16.

skratchdot commented 6 years ago

Sorry. I somehow missed this. Thanks for the great explaination @South-Paw (and the confirmation @ttessarolo)! I haven't really used this project for a long time, so haven't done a very good job at maintaining it. I accept PR's, but will also try to fix this myself if I get some free time on a weekend to work on it.

ttessarolo commented 6 years ago

@skratchdot thanks for your attention and for your time! please let us know if something is gonna change on the official release.

jayna456 commented 4 years ago

@South-Paw , I followed your suggestions but I am still getting 2 ul, Is there anything to do after comment out that line???