-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Lens] Make the dimension flyout panel stay close on outside click #83059
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
I tested this PR and it fixes the issue, but why is it happening in the first place? Where are these I debugged a little and it seems like they are triggered by the |
I suspected they are related at some level. |
Note the latest change introduce a small delay on panel closing (150ms). While the number does not seem too big, the delay (even if small) is noticeable. |
@elasticmachine merge upstream |
setTimeout(() => { | ||
handleClose(); | ||
setFocusTrapIsEnabled(false); | ||
}, 150); // <= 150 has been chosen from empirical testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really concerned about this approach. As you said it will add a delay as well as not even work in all cases depending on the timing of things.
I think I wasn't really clear in my original review, I think it's fine to use the updater function setState
approach to make sure the flyout stays closed if the additional setStates triggered on closing the popover are really necessary, but I was under the impression the state update happening in this scenario doesn't even make sense and we can just avoid it all together.
So, in summary
- Let's please not introduce constant waiting times in the UI if avoidable
- See if it's possible to not update the state on unmount like this
- If we have to, use the updater function to make sure the flyout stays closed, but limit the cases where this is happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move the issue upstream and have EUI fix it at component level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created issue elastic/eui#4265 on EUI repo and will remove the code here.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in Chrome and Firefox, works as expected.
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…lastic#83059) Co-authored-by: Kibana Machine <[email protected]>
…ick (#83059) (#83669) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
…ode-details * 'master' of github.com:elastic/kibana: Remove dependency of tests on strict SyntaxKind values (elastic#83440) [SecuritySolution] override timerange for prebuilt templates (elastic#82468) [Enterprise Search] Added a shouldShowActiveForSubroutes option (elastic#83338) [Lens] Make the dimension flyout panel stay close on outside click (elastic#83059) [Security Solution] Gracefully handle errors in detection rules install (elastic#83306) Fix advanced settings category sorting (elastic#83394)
Summary
This PR addresses a bug identified in #82455 : if a popover inside the flyout was open, then clicking outside the panel would close first both popover and flyout, then quickly reopen the latter.
Why that happen? That's because on flyout panel close multiple updates happen at the same time (well, sequentially), and in particular multiple calls to thesetActiveDimension
state update were triggered: in particular one calls would refresh the state adding a tiny partialisNew: false
property, while re-using the previous state.Due to the multiple updates and React didn't have time to redraw the component, the
activeDimension
variable used was stale, causing the reopening: using the functional version of thesetActiveDimension
hook made it use the "last" version of the state in order to update.Why that happened (2)? The main issue here was the double mechanism to toggle the popover, together with some state updates happening.
Removed the state update on close, the popover were still visible for a fraction of a second on the top left of the screen (easy to check with the
Filter
operation popover). While this seemed a minor issue it was only the effect of a bigger problem: the way the internal popover was closed was not correct, as it had to make sure to notify the parent component first, then its own state. On top of this beautiful dnd did not have the time to unmount in some scenarios showing a pretty big warning + error in development.The fix has addressed in this PR only the popover close commands order, leaving the mount/unmount problem. A new issue on the EUI repo has been created to address this problem upstream.
The solution was to have the popover correctly check its state and close only via the correct call, and delay a bit the flyout panel closing in order to give time to the popover and beautiful dnd to unmount correctly. The150
delay has been empirically measured on my machine and it was the smallest value by which the visual glitch was not visible.Not really sure on how to test this (maybe functional?). Any suggestion is welcome.
Fixes: #82455