fix(Modal|Popup|Portal): fix usage of eventStack sub/unsub #2099

austinfox · 2017-09-21T22:42:58Z

fixes #2075

Without this change, any event handlers added to EventStack under the 'Portal' namespace will be appended to an array. If another Portal is then mounted, new event handlers will be added to the EventStack and take precedence over the original portal's event handlers.

This change allows each portal to store its event handlers under its own namespace in EventStack, so when you click from one portal to another, the first one is appropriately closed.

codecov-io · 2017-09-21T22:50:36Z

Codecov Report

Merging #2099 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2099      +/-   ##
==========================================
+ Coverage   99.76%   99.76%   +<.01%     
==========================================
  Files         149      149              
  Lines        2598     2599       +1     
==========================================
+ Hits         2592     2593       +1     
  Misses          6        6

Impacted Files	Coverage Δ
src/addons/Portal/Portal.js	`100% <100%> (ø)`	⬆️
src/modules/Modal/Modal.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 e2fc8f5...f217ef9. Read the comment docs.

layershifter

@austinfox thanks for picking up this 👍

Proposed changes useless, because in fact it's equals to default pool of the eventStack. My initial proposal about the exclusive prop is also not ideal.

Problem

We have a couple of bugs with multiple Modals, eventStack solves them and improves performance.

What eventStack actually does? It has an entity called pool. Event handlers that included in default pool will be fired usual. But, only the last added event handler to the named pool will be fired when corresponding event will be occured.

You can open this Modal example. Open both modals and then press Esc key. Only top modal will be closed and it's correct. But, as we can see it causes problems for Popups.

Solution

So, my idea is to add the eventPool prop to the Portal which defaults to default:

const { eventPool } = this.props

eventStack.sub('keydown', this.handleEscape, eventPool)

This prop should be also added to Modal and Popup, where it will be defaults to Modal and default accordingly. It will keep Modal working correctly and solve the problem with event propagation in Popup.

Let me know what you think.

austinfox · 2017-09-22T15:11:04Z

@layershifter Yea I was thinking about adding a prop to Modal, Popup, and Portal. I'm not convinced it will be clear to others how to prevent this bug though even if we add eventPool as a prop. If they have an in-depth knowledge about the inner workings of Portal and eventStack, then yes they will be able to fix the problem. But by default, clicking on a popup and then clicking on another popup will be broken. Why not default to having each Popup use its own eventPool? I think that is standard behavior, whenever you click away from a popup is should almost always close.

austinfox · 2017-09-22T15:16:20Z

@layershifter ignore my last comment. Taking a look at the EventStack code, I now understand what you mean. When the 'default' poolName is used, all handlers are called for that event. This will work just fine, I'll modify the PR in a few hours. Is there any way we can get this change into a new version of SUI react asap? My team site is currently breaking for IE users because we cannot use this feature. I had to rely on='focus' and do some other hacks that work well for chrome, Firefox, safari, and edge, but... sigh, IE...

layershifter

Looks good to me. I left some comments about styling.

layershifter · 2017-09-22T19:49:17Z

src/addons/Portal/Portal.js

@@ -115,6 +115,9 @@ class Portal extends Component {
    /** Controls whether or not the portal should open when mousing over the trigger. */
    openOnTriggerMouseEnter: PropTypes.bool,

+    /** Event pool namespace that is used to handle component events */
+    eventPool: PropTypes.string,


Please add prop in alphabetical order.

didn't know about this, done :)

layershifter · 2017-09-22T19:49:24Z

src/addons/Portal/Portal.js

@@ -126,6 +129,7 @@ class Portal extends Component {
    closeOnDocumentClick: true,
    closeOnEscape: true,
    openOnTriggerClick: true,
+    eventPool: 'default',


layershifter · 2017-09-22T19:49:43Z

src/addons/Portal/Portal.js

@@ -374,6 +378,7 @@ class Portal extends Component {
    const {
      mountNode = isBrowser ? document.body : null,
      prepend,
+      eventPool,


layershifter · 2017-09-22T19:50:46Z

src/addons/Portal/Portal.js

@@ -394,6 +399,8 @@ class Portal extends Component {

    debug('unmountPortal()')

+    const { eventPool } = this.props


New line before this declaration is useless.

layershifter · 2017-09-22T19:56:10Z

When the 'default' poolName is used, all handlers are called for that event.

Exactly 👍

Is there any way we can get this change into a new version of SUI react asap?

I will perform style changes tomorrow, if you will not do them before me 😄 I want to hear there @levithomason, too. I think that we can include it to next release on this weekend.

levithomason · 2017-09-23T17:17:43Z

I've been following this issue/PR and I'm happy with this result 👍. Thanks for hashing it out guys.

levithomason · 2017-09-24T18:25:53Z

Released in semantic-ui-react@0.74.0

drewsbrew · 2017-11-14T00:56:06Z

Looking at the multiple modals example in the docs for v0.76, a single press of the Escape key closes both modals. I am seeing the same behavior in my project even with unique eventPool props values. Has this regressed or am I missing something?

brianespinosa · 2017-11-14T23:36:56Z

@drewsbrew If that is what you are experiencing, please open up a separate bug report form with a codesandbox example.

layershifter · 2017-11-18T19:15:51Z

@drewsbrew Bug actually exists, however @brianespinosa please create new issues instead of necroposting.

Will be fixed in #2329.

Fix for Portal usage of EventStack sub/unsub

1b02695

austinfox mentioned this pull request Sep 21, 2017

eventStack: Multiple Popups do not behave correctly #2075

Closed

layershifter requested changes Sep 22, 2017

View reviewed changes

layershifter changed the title ~~Fix for Portal usage of EventStack sub/unsub~~ fix(Modal|Popup|Portal): if usage of eventStack sub/unsub Sep 22, 2017

layershifter changed the title ~~fix(Modal|Popup|Portal): if usage of eventStack sub/unsub~~ fix(Modal|Popup|Portal): fix usage of eventStack sub/unsub Sep 22, 2017

layershifter added the needs author feedback label Sep 22, 2017

austinfox added 2 commits September 22, 2017 09:15

Responding to PR comments

24ee33c

Fixing lint errors

dd1dfab

layershifter requested changes Sep 22, 2017

View reviewed changes

Addressing PR style comments

f217ef9

layershifter added ready for review and removed needs author feedback labels Sep 22, 2017

layershifter approved these changes Sep 22, 2017

View reviewed changes

levithomason merged commit de091e6 into Semantic-Org:master Sep 23, 2017

austinfox deleted the portal-eventstack-bug branch September 25, 2017 20:27

layershifter mentioned this pull request Nov 18, 2017

fix(Portal): fix usage of eventPool #2329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Modal|Popup|Portal): fix usage of eventStack sub/unsub #2099

fix(Modal|Popup|Portal): fix usage of eventStack sub/unsub #2099

austinfox commented Sep 21, 2017 •

edited

Loading

codecov-io commented Sep 21, 2017 •

edited

Loading

layershifter left a comment

austinfox commented Sep 22, 2017

austinfox commented Sep 22, 2017

layershifter left a comment

layershifter Sep 22, 2017

austinfox Sep 22, 2017

layershifter Sep 22, 2017

layershifter Sep 22, 2017

layershifter Sep 22, 2017

layershifter commented Sep 22, 2017

levithomason commented Sep 23, 2017

levithomason commented Sep 24, 2017

drewsbrew commented Nov 14, 2017

brianespinosa commented Nov 14, 2017

layershifter commented Nov 18, 2017

		@@ -394,6 +399,8 @@ class Portal extends Component {

		debug('unmountPortal()')

		const { eventPool } = this.props

fix(Modal|Popup|Portal): fix usage of eventStack sub/unsub #2099

fix(Modal|Popup|Portal): fix usage of eventStack sub/unsub #2099

Conversation

austinfox commented Sep 21, 2017 • edited Loading

codecov-io commented Sep 21, 2017 • edited Loading

Codecov Report

layershifter left a comment

Choose a reason for hiding this comment

Problem

Solution

austinfox commented Sep 22, 2017

austinfox commented Sep 22, 2017

layershifter left a comment

Choose a reason for hiding this comment

layershifter Sep 22, 2017

Choose a reason for hiding this comment

austinfox Sep 22, 2017

Choose a reason for hiding this comment

layershifter Sep 22, 2017

Choose a reason for hiding this comment

layershifter Sep 22, 2017

Choose a reason for hiding this comment

layershifter Sep 22, 2017

Choose a reason for hiding this comment

layershifter commented Sep 22, 2017

levithomason commented Sep 23, 2017

levithomason commented Sep 24, 2017

drewsbrew commented Nov 14, 2017

brianespinosa commented Nov 14, 2017

layershifter commented Nov 18, 2017

austinfox commented Sep 21, 2017 •

edited

Loading

codecov-io commented Sep 21, 2017 •

edited

Loading