search

shentao / vue-multiselect

Universal select/multiselect/tagging component for Vue.js
https://vue-multiselect.js.org/
MIT License
6.73k stars 993 forks source link

Replace display:none inline styles with classes #284

Open wujekbogdan opened 7 years ago

wujekbogdan commented 7 years ago

First of all, thanks for implementing this. Thanks to this (and because of BEM syntax) writing custom styles is much easier and cleaner.

I have a little improvement proposal. It would be perfect if you replaced inline styles with classes. For example multiselect__content when it's hidden it has adisplay:none inline style. It would be better if it has a BEM modifier, a hidden element could have multiselect__content multiselect__content--hidden classes (.multiselect__content--hidden selector would have display:none then). Thanks to this it would be possible to write custom animation styles for these elements. For example, it would be possible to write a nice fade-in/fade-out effect for the dropdown, or even more fancy animations in pure CSS.

I think it's a safe change, it wouldn't cause any side-effects and/or incompatibilities.

What do you think?

shentao commented 7 years ago

Hey! Yeah I think that would be safe to implement.

However, there is just one potential drawback – currently we’re using the v-show (display: none), but for performance reasons it is better to use v-if. With v-show the component might slow down when you have multiple dropdowns with around 200 options in each of them. That at least was my case when using the 1.x version. On the otherside, there also were some problems regarding scrolling with IE10 when using v-if and that’s why I changed it to v-show. Maybe before we add the classes, we could test if in v2 there are still problems with v-if on IE. Even if we were to change to v-if then, we could still make it possible to have nice CSS animations by attaching a Vue transition.

What do you think about this?