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

Use event delegation #644

Merged
merged 1 commit into from
Oct 15, 2019
Merged

Conversation

bronzehedwick
Copy link

@bronzehedwick bronzehedwick commented Sep 27, 2019

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@bronzehedwick
Copy link
Author

bronzehedwick commented Sep 30, 2019

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.

[22:15:14]  Verifying Cypress can run /home/travis/.cache/Cypress/3.1.5/Cypress [started]

[nodemon] 1.18.10

[nodemon] to restart at any time, enter `rs`

[nodemon] watching: *.*

[nodemon] starting `npm run css:build`

[22:15:15]  Verifying Cypress can run /home/travis/.cache/Cypress/3.1.5/Cypress [failed]

Cypress failed to start.

EDIT

This same issue is happening on several other PRs in this repo as well.

@jshjohnson
Copy link
Collaborator

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
@guidefreitas
Copy link

This PR attenuate some of performance issues we are having when a select has a lot of options.

Thanks!

@bronzehedwick
Copy link
Author

If you merge in latest master, the CI build should be fixed 👍

I merged in master and the builds now pass. Thanks!

@bronzehedwick
Copy link
Author

@jshjohnson any chance this PR can get a review? Thanks!

@jshjohnson jshjohnson merged commit e7d775e into Choices-js:master Oct 15, 2019
@jshjohnson
Copy link
Collaborator

Thanks @bronzehedwick 👍

@markx3
Copy link

markx3 commented Oct 15, 2019

Awesome! 🎉

@jshjohnson
Copy link
Collaborator

This fix is included in version 7.0.2 👍

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.

Use event delegation?
4 participants