feat(Portal): More flexible configuration #590
Conversation
50b2cbc to
77e7fed
Compare
Current coverage is 100% (diff: 100%)@@ master #590 diff @@
====================================
Files 119 119
Lines 1894 1915 +21
Methods 0 0
Messages 0 0
Branches 0 0
====================================
+ Hits 1894 1915 +21
Misses 0 0
Partials 0 0
|
|
@levithomason - this is blocking the |
|
On it, thanks! |
|
@jcarbo, I pulled your branch into the Popup and noticed that the first time the Popup shows up the timer on leave isn't respected. The bug goes away every time I hover the trigger after that. |
| @@ -37,15 +37,27 @@ class Portal extends Component { | |||
| /** Controls whether or not the portal should close on blur of the trigger. */ | |||
| closeOnTriggerBlur: PropTypes.bool, | |||
|
|
|||
| /** Controls whether or not the portal should close when mousing out of the trigger. */ | |||
| closeOnTriggerMouseLeave: PropTypes.bool, | |||
| /** Controls whether or not the portal should close on blur of the trigger. */ | |||
There was a problem hiding this comment.
close on blur != closeOnTriggerClick
| * Controls whether or not the portal should close when mousing out of the | ||
| * trigger OR the portal content. | ||
| */ | ||
| closeOnMouseLeave: PropTypes.bool, |
There was a problem hiding this comment.
Hm, seems like we should separate these. I know the names are hideous, but I can see wanting one or the other:
closeOnTriggerMouseLeave
closeOnPortalMouseLeave
Thoughts?
There was a problem hiding this comment.
That was my initial thought, but then I was wondering how do you configure allowing mousing from the trigger to the portal? I think I just figured it out though:
- If only
closeOnTriggerMouseLeaveis set, then mouseover/leave on the portal won't do anything - If
closeOnPortalMouseLeaveis set, then mouseover of the portal clears the timer, mouseleave restarts the timer.
So closeOnPortalMouseLeave has the added effect of preventing close when mousing over the gap from trigger -> portal
This actually lets us keep more closely with SUI's hoverable:
hoverable => falseis{ closeOnTriggerMouseLeave: true }hoverable => trueis{ closeOnTriggerMouseLeave: true, closeOnPortalMouseLeave: true }
There was a problem hiding this comment.
I'm moving quickly here, but I think this makes sense.
| @@ -229,6 +273,12 @@ class Portal extends Component { | |||
| this.trySetState({ open: false }) | |||
| } | |||
|
|
|||
| closeWithTimeout = (e, delay) => { | |||
| // React wipes certain props (e.g. currentTarget) so we need to clone. | |||
| const eventClone = { ...e } | |||
There was a problem hiding this comment.
Heads up, React wipes the entire event object, nullifying all values. That's because React reuses a single event object. In order to persist the event object for async access, use e.persist():
https://facebook.github.io/react/docs/events.html#event-pooling
I think what is here will technically work, but it still seems as though we should be using e.persist(), even if we're copying the persisted object.
There was a problem hiding this comment.
Tried e.persist initially but doesn't preserve the currentTarget, see facebook/react#2857. This is kinda gross but it's the only way to make a copy of the event as-is.
There was a problem hiding this comment.
Womp womp, thanks for the link
|
@jcarbo Nevermind the issue I reported earlier (aka. timer not respected the 1st time), I can't reproduce it anymore. |
* Add closeOnTriggerClick prop * Update closeOnMouseLeave to handle mouse leave of either trigger or portal * Add configurable delay for open/close on hover
1a5763f to
f65de27
Compare
|
Ok made the changes discussed here and checks came back green. I think this is 👍 |
|
Released in |
closeOnTriggerClickpropcloseOnMouseLeaveto handle mouse leave of either trigger or portalThis addresses an open issue of #570 which is not being able to mouse-over from the trigger of a
Popup.