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

fix(EventStack): fix compatibility with IE11 #3124

Merged
merged 7 commits into from
Sep 10, 2018
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Sep 4, 2018

Fixes #3043.


What was done?

Move to a separate repo

EventStack was moved to separate package and was published as @semantic-ui-react/event-stack.

Motivation: this package requires a specific test config for browsers. However, I'm not happy with this decision, I prefer monorepos 😺 Possible, Stardust will adopt it.

Browser compability

EventStack is now fully compatible with IE11. To avoid regresions I added browser tests:

Performance 🚀

I and my сolleagues carefully profiled this part of the lib, performance was drasticaly improved. The most slow was EventSet:

Feel free to edit benchmarks and propose more performant solution.


There I also couple serious bugs with EventStack event handling, I will concentrate on them after merge. I'm also planning to move #2990 to this package.

@layershifter layershifter changed the title fix(EventStack): fix compatibility with IE11 [WIP] fix(EventStack): fix compatibility with IE11 Sep 4, 2018
@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

Merging #3124 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3124   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files         163      163           
  Lines        2742     2742           
=======================================
  Hits         2740     2740           
  Misses          2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d0688a...7c0e4b3. Read the comment docs.

@layershifter layershifter changed the title [WIP] fix(EventStack): fix compatibility with IE11 fix(EventStack): fix compatibility with IE11 Sep 7, 2018
@levithomason
Copy link
Member

Possible, Stardust will adopt it.

Yes please!

@levithomason
Copy link
Member

We have a fork of this in Stardust as well. Yes, I would love to have a single utility here. It is also duplicated in Fabric (Microsoft). One of the goals of Stardust is to normalize these kinds of utilities between frameworks. Let's merge this for now, then we can port it to Stardust and publish it as a monorepo.

There might still be a bug here, not sure. Here's what I think I've found. I noticed the bug in #3062 goes away if we fix the sub method. Currently, the entire pool is removed then re-added on sub. However, the pool shares handlers with other components under the same pool name. When a second Dropdown opens it calls sub for close on document click, which removes the entire pool (including the first Dropdown's close on document click), then adds only the second Dropdown's close on document click.

When I use document.addEventListener instead, all works as expected. When I remove the line in EventStack that removes the entire pool on sub, all works fine as well. So, I think we need to skip sub if that handler already exists and not remove/add the entire pool.

@levithomason levithomason merged commit b5c76fb into master Sep 10, 2018
@levithomason levithomason deleted the fix/stack-ie11 branch September 10, 2018 19:55
@levithomason
Copy link
Member

Released in [email protected].

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.

3 participants