fabianlindfors / multi.js

A user-friendly replacement for select boxes with the multiple attribute enabled
https://fabianlindfors.se/multijs
MIT License
953 stars 76 forks source link

Use one event listener per select instead of one per option #13

Closed MiguelSanchezGonzalez closed 7 years ago

MiguelSanchezGonzalez commented 7 years ago

Hi,

Here I'm adding all the click and keypress event listeners to the main wrapper instead of having them on the single options. This will reduce the amount of event listeners in the page plus remove work of the refresh function as is not declaring listeners anymore every time the render.

Here you can see the difference of event listeners attached to the page between master and my branch after performing several searches:

master

branch-master Reach 200+ listeners

feature/single-event-listeners

branch-feature-single-event-listeners Keeps the same amount all the time

Also removes the IIFE as is not needed anymore in the render loop.

As 'side effect', now if you click on one of the already selected options under the non-selected-wrapper the state of that option will be toggled as well. I think that's the expected behaviour as is how a multiple select box works by default.

This could be disabled by the users setting something like this on their css:

.multi-wrapper .non-selected-wrapper .item.selected {
    opacity: .5;
    pointer-events: none;
}

But I still think is something useful :)

Try it out yourself and let me know what do you think

Thanks!

fabianlindfors commented 7 years ago

This is really cool Miguel!

I'm going to check it out soon when I find some time but seems like a huge improvement. Great work!

fabianlindfors commented 7 years ago

Just tried it out and it seems to work really well.

Thanks a lot for contributing!

MiguelSanchezGonzalez commented 7 years ago

Sweet!

I'd like to keep contributing, are you working on any of the currently open issues?

otherwise could have a look to any of this two:

Let me know :)

fabianlindfors commented 7 years ago

Hi again!

I'm awfully sorry for replying so late. I have some plans for how #8 is going to work so I'd prefer to get started on that on my own. #2 on the other hand would be great if you'd like to contribute.

I'm thinking that it should work similar to select2 where one can provide a callback for the search function. Only the selected options are then saved to the underlying select element.

What do you have in mind?

MiguelSanchezGonzalez commented 7 years ago

Hi Fabian,

No bothers I also have limited time to work on open source projects :)

I had more or less the same idea, I have implemented it on this branch:

https://github.com/MiguelSanchezGonzalez/multi.js/tree/feature/ajax-options

have a look to it and let me know what do you think.

The feature is already documented on the README.md so you can get a quick overview of what's going on and how to use it :)

Cheers!

fabianlindfors commented 7 years ago

Great work, Miguel!

I haven't had the time to test it out but from a quick glance it looks quite good. A few thoughts:

I hope you understand my concerns!

MiguelSanchezGonzalez commented 7 years ago

Hi Fabian, thanks for the feedback!

Sure, that TS docs are more focused for you to understand what was going on a bit faster :) I'll trim it down once the feature is done!

Regarding the AJAX, do you have any specific browser version ranges in mind? or just a regular XMLHttpRequest will do the work?

I'll drop you a comment as soon as I have a few minutes to work on that :+1:

Thanks again!

fabianlindfors commented 7 years ago

I haven't really decided what browsers multi.js should support and truthfully I haven't done any extensive browser testing yet (it's on the todo list). I've been trying to keep the Javascript as basic and safe as possible to maximize compatibility. My goal is to create a library which one should just be able to drop into any project and it should work flawlessly with minimal configuration.

With that in mind a XMLHttpRequest will do the job just fine. Keep up the great work!