[EuiFlyout] Remain open when mousedown happens inside the flyout
#5810
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5810/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5810/ |
|
Going to run CI on this 2 more times to try and catch any flakiness |
|
jenkins test this |
|
|
||
| // If ownFocus is set, wrap with an overlay and allow the user to click it to close it. | ||
| if (ownFocus && !isPushed) { | ||
| if (isDefaultConfiguration) { |
There was a problem hiding this comment.
[super optional comment] This var name doesn't say very much to me at first glance - I find the original condition more descriptive. Since it's really only used in two places I'd honestly lean towards either leaving it unabstracted or trying to find a clearer name, e.g. shouldCloseOnOverlayClick
| const onClickOutside = | ||
| (isDefaultConfiguration && outsideClickCloses !== false) || | ||
| outsideClickCloses === true | ||
| ? onClose | ||
| : undefined; |
There was a problem hiding this comment.
Also super optional feedback, but this is pretty confusing to read - I wonder if a useMemo with early returns would be easier to parse?
| const onClickOutside = | |
| (isDefaultConfiguration && outsideClickCloses !== false) || | |
| outsideClickCloses === true | |
| ? onClose | |
| : undefined; | |
| const onClickOutside = useMemo(() => { | |
| if (outsideClickCloses === false) return undefined; | |
| if (outsideClickCloses === true) return onClose; | |
| if (isDefaultConfiguration) return onClose; | |
| return undefined; | |
| }, [outsideClickCloses, isDefaultConfiguration]); |
After rereading again I'm not super sure this is a huge improvement so feel free to disregard this lol
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5810/ |
|
2nd CI run passed, running again - jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5810/ |
cee-chen
left a comment
cee-chen
left a comment
There was a problem hiding this comment.
I know you're busy with Emotion stuff so just going to go ahead and approve so we have this fix in for next release!
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5810/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5810/ |
3 participants
Summary
Fixes a bug where in an
EuiFlyoutwith an overlay mask (default config) would be closed when a click event hadmousedowninside the flyout andmouseupoutside the flyout (e.g., click, drag, and release where release is outside the flyout).The cause was
EuiOverlayMaskfiring itsonClickhandler (in this case to close the flyout) onmouseup. There's a lot going on with this, but the gist is that browsers don't align on what element to designate astargetwhenmousedownandmouseuporiginations don't match.To solve the problem, I opted to simplify the component composition inside
EuiFlyoutand allowEuiFocusTrapto handle outside click detection in all cases, rather than haveEuiOverlayMaskbe responsible. This also allowed for refactoring to also haveEuiFocusTrapnegate the need forEuiOutsideClickDetector.Before
https://user-images.githubusercontent.com/2728212/163847290-bcbb3543-2e0e-49a2-8a63-ef5699a6128c.mov
After
https://user-images.githubusercontent.com/2728212/163847313-398c4845-c8c6-41bd-8365-ad282585951f.mov
Checklist