-
Notifications
You must be signed in to change notification settings - Fork 618
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
Use event delegation #644
Use event delegation #644
Conversation
The tests are failing for this PR, but it's because cypress fails to start. I can't think of any code changes here that would cause that. And funny enough I'm seeing a similar issue with cypress failing to start for a work project. Is there some larger issue with cypress going on? Here's the relevant log output.
EDITThis same issue is happening on several other PRs in this repo as well. |
If you merge in latest master, the CI build should be fixed 👍 |
Instead of attaching a new root-level event listener for bubbling events for every choices instance, use a simple event delegation script to handle each event type. Each event callback function already is coded as if it were fully delegated, since the events are attached at the document level, so no changes are needed to detect which element is being called. Note that focus and blur event do not bubble, so they have been left as they are. Also note that the event delegation uses an IIFE purposely instead of ES6 modules, since the event list should be globally cached, and it doesn't make sense to instantiate a new scope for each instance (then we're back where we started!) fix Choices-js#643
7631d2a
to
acb8962
Compare
This PR attenuate some of performance issues we are having when a select has a lot of options. Thanks! |
I merged in master and the builds now pass. Thanks! |
@jshjohnson any chance this PR can get a review? Thanks! |
Thanks @bronzehedwick 👍 |
Awesome! 🎉 |
This fix is included in version 7.0.2 👍 |
Description
Instead of attaching a new root-level event listener for bubbling events
for every choices instance, use a simple event delegation script to
handle each event type.
Each event callback function already is coded as if it were fully
delegated, since the events are attached at the document level, so
no changes are needed to detect which element is being called.
Note that focus and blur event do not bubble, so they have been left as
they are.
Also note that the event delegation uses an IIFE purposely instead of ES6 modules, since the event list should be globally cached, and it doesn't make sense to instantiate a new scope for each instance (then we're back where we started!)
Motivation and Context
Using event delegation increase page speed, especially if there are many choices instances.
fix #643
How Has This Been Tested?
I ran the standard tests via
npm test
on macOS 10.13.6.I also tested the local server manually to make sure I could still perform all operations.
This change's scope is limited to adding an event delegation script and the
_addEventListeners
and_removeEventListeners
functions.Types of changes
Checklist: