feat(events): add eventStack #1733

layershifter · 2017-06-02T12:55:41Z

What is it?

This PR introduces a new event handling system that will solve following problems:

event priority, only top event will be emitted (will help with multiple Modals and Dropdowns);
solve performance problems with multiple Dropdowns;

Example

-document.addEventListener('keydown', this.openOnArrow)
-document.addEventListener('keydown', this.openOnSpace)
+eventStack.sub('keydown', [this.openOnArrow, this.openOnSpace])

-document.removeEventListener('keydown', this.openOnArrow)
-document.removeEventListener('keydown', this.openOnSpace)
-document.removeEventListener('keydown', this.moveSelectionOnKeyDown)
-document.removeEventListener('keydown', this.selectItemOnEnter)
-document.removeEventListener('keydown', this.removeItemOnBackspace)
+eventStack.unsub('keydown', [
+  this.openOnArrow,
+  this.openOnSpace,
+  this.moveSelectionOnKeyDown,
+  this.selectItemOnEnter,
+  this.removeItemOnBackspace,
+])

layershifter · 2017-06-02T12:57:17Z

@levithomason Would be awesome to hear your throughts.

However, I want to finish my other open PRs before I'll start there.

dvdzkwsk · 2017-06-07T13:54:40Z

src/lib/eventPool.js

+
+  unlisten = name => {
+    if(_.some(this.queues, name)) return
+    document.removeEventListener(name, this.emit(name))


The emit method returns a new function on each call. Are you sure this removes the original listener(s)?

@davezuko Yep, you're fully right there. I pushed some changes and tests, now it works as I described.

codecov-io · 2017-06-07T14:53:37Z

Codecov Report

Merging #1733 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1733      +/-   ##
=========================================
- Coverage    99.8%   99.8%   -0.01%     
=========================================
  Files         148     148              
  Lines        2589    2555      -34     
=========================================
- Hits         2584    2550      -34     
  Misses          5       5

Impacted Files	Coverage Δ
src/modules/Search/Search.js	`100% <100%> (ø)`	⬆️
src/addons/Portal/Portal.js	`100% <100%> (ø)`	⬆️
src/modules/Dropdown/Dropdown.js	`100% <100%> (ø)`	⬆️

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 23ec465...78564ee. Read the comment docs.

layershifter · 2017-06-08T08:35:58Z

src/modules/Dropdown/Dropdown.js

      }
-      document.addEventListener('keydown', this.removeItemOnBackspace)


moveSelectionOnKeyDown, selectItemOnEnter and removeItemOnBackspace should be added only if the Dropdown is open

removeItemOnBackspace works when a multiple search selection Dropdown is focused and openOnFocus={false}. The cursor focusses the Dropdown, it does not open, but the backspace listener is added so the user can remove labels:

I'd prefer we don't change any logic in this PR except the adding/removing events. That said, it looks like we're missing a test for this as well. Changing that logic should have broken a test.

@levithomason Thanks for clarification, I've restored event handlers and added a fix. Now eventPull handles only unique events

layershifter

@davezuko @levithomason I've performed final updates there, will be awesome to get a feedback there.

…tic-Org/Semantic-UI-React into feat/events

…React into feat/events

levithomason · 2017-07-21T15:23:54Z

I believe this also solves #1157, correct?

layershifter · 2017-07-21T19:19:06Z

No, #1157 will need an another solution, however this PR opens the road to it.

…React into feat/events

levithomason · 2017-08-20T17:39:40Z

I have not gone into this in detail yet, but I don't want to hold it up any longer so I'd like to merge it.

One nit I have is that a pool, in my experience, is a set of shared resources where any available item may be pulled form the pool (like a db connection). I'd like to call this a "stack", which indicates they will be processed one at a time ~~in the order they were added~~ from the end. Is this accurate with the implementation here?

If so, let's go with eventStack over eventPool, merge, and move forward 👍

EDIT Fix stack description, thanks David.

layershifter · 2017-08-20T19:24:13Z

I will change tomorrow and will sync with master 👍

…React into feat/events

layershifter · 2017-08-21T09:15:06Z

I merged with master, renamed to eventStack and fixed lint issues.

dvdzkwsk · 2017-08-21T12:12:09Z

I'd like to call this a "stack", which indicates they will be processed one at a time in the order they were added. Is this accurate with the implementation here?

I think you mean a queue ;) <3

kyrasteen · 2017-08-24T21:22:22Z

Hey there. Thanks for fixing this issue. I was wondering if this will be merged and released in the near future? It would help my team out with a current production bug. If there is anything I can contribute to get this merged, happy to help. :)

levithomason · 2017-08-27T22:17:51Z

I think you mean a queue ;) <3

Whoops, I still mean a stack, however, I was lazy and inaccurate in the description. I meant to say "processed one at a time, from the end" rather than "in the order added".

When I imagine a Popup opening inside of a Modal, I think of a stack of handlers. The Popup handlers should be processed first even though they were added last. In a queue, I'd think the Modal handlers would be processed first, then the Popup (in the order they were added).

Thanks for pointing out the error @davezuko.

levithomason · 2017-08-28T00:51:29Z

Released in [email protected]

feat(events): add eventPool

948dbf7

layershifter added enhancement optimization labels Jun 2, 2017

dvdzkwsk reviewed Jun 7, 2017

View reviewed changes

feat(events): add eventPool

051bdf2

Alexander Fedyashov added 2 commits June 7, 2017 18:44

feat(events): update components

b96b1b1

feat(events): update components

1f90651

layershifter added the ready for review label Jun 8, 2017

layershifter commented Jun 8, 2017

View reviewed changes

feat(events): merge with master

d0ab43a

layershifter force-pushed the feat/events branch from 2707a7d to d0ab43a Compare June 26, 2017 09:33

layershifter and others added 2 commits July 9, 2017 12:51

Merge branches 'feat/events' and 'master' of https://github.com/Seman…

1aecccc

…tic-Org/Semantic-UI-React into feat/events

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

28de78c

…React into feat/events

layershifter mentioned this pull request Jul 18, 2017

feat(Responsive): add component #1872

Merged

layershifter added needs author feedback and removed ready for review labels Jul 31, 2017

Alexander Fedyashov added 2 commits July 31, 2017 20:13

feat(eventPool): restore handlers

0935570

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

59ee039

…React into feat/events

layershifter added ready for review and removed needs author feedback labels Jul 31, 2017

Alexander Fedyashov added 2 commits July 31, 2017 20:52

feat(eventPool): only unique handlers

ffa4a43

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

21445e1

…React into feat/events

levithomason added needs author feedback and removed ready for review labels Aug 20, 2017

Alexander Fedyashov added 2 commits August 21, 2017 11:58

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

6b49fec

…React into feat/events

refactor(mixed): rename eventPool to eventStack

e83f4bb

layershifter changed the title ~~feat(events): add eventPool~~ feat(events): add eventStack Aug 21, 2017

style(eventStack): fix lint issues

78564ee

layershifter added ready for review and removed needs author feedback labels Aug 21, 2017

levithomason merged commit 3544d5a into master Aug 27, 2017

levithomason deleted the feat/events branch August 27, 2017 22:19

levithomason mentioned this pull request Aug 27, 2017

Dismissable Message Component inside a Popup closes the Popup itself #1580

Closed

mhagmajer mentioned this pull request Oct 24, 2017

Bug on Dropdown.componentWillUnmount() #2120

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(events): add eventStack #1733

feat(events): add eventStack #1733

layershifter commented Jun 2, 2017 •

edited

Loading

layershifter commented Jun 2, 2017 •

edited

Loading

dvdzkwsk Jun 7, 2017

layershifter Jun 7, 2017

codecov-io commented Jun 7, 2017 •

edited

Loading

layershifter Jun 8, 2017

levithomason Jul 28, 2017

layershifter Jul 31, 2017

layershifter left a comment

levithomason commented Jul 21, 2017

layershifter commented Jul 21, 2017

levithomason commented Aug 20, 2017 •

edited

Loading

layershifter commented Aug 20, 2017

layershifter commented Aug 21, 2017

dvdzkwsk commented Aug 21, 2017 •

edited

Loading

kyrasteen commented Aug 24, 2017

levithomason commented Aug 27, 2017

levithomason commented Aug 28, 2017

		}
		document.addEventListener('keydown', this.removeItemOnBackspace)

feat(events): add eventStack #1733

feat(events): add eventStack #1733

Conversation

layershifter commented Jun 2, 2017 • edited Loading

What is it?

Example

layershifter commented Jun 2, 2017 • edited Loading

dvdzkwsk Jun 7, 2017

Choose a reason for hiding this comment

layershifter Jun 7, 2017

Choose a reason for hiding this comment

codecov-io commented Jun 7, 2017 • edited Loading

Codecov Report

layershifter Jun 8, 2017

Choose a reason for hiding this comment

levithomason Jul 28, 2017

Choose a reason for hiding this comment

layershifter Jul 31, 2017

Choose a reason for hiding this comment

layershifter left a comment

Choose a reason for hiding this comment

levithomason commented Jul 21, 2017

layershifter commented Jul 21, 2017

levithomason commented Aug 20, 2017 • edited Loading

layershifter commented Aug 20, 2017

layershifter commented Aug 21, 2017

dvdzkwsk commented Aug 21, 2017 • edited Loading

kyrasteen commented Aug 24, 2017

levithomason commented Aug 27, 2017

levithomason commented Aug 28, 2017

layershifter commented Jun 2, 2017 •

edited

Loading

layershifter commented Jun 2, 2017 •

edited

Loading

codecov-io commented Jun 7, 2017 •

edited

Loading

levithomason commented Aug 20, 2017 •

edited

Loading

dvdzkwsk commented Aug 21, 2017 •

edited

Loading