-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat: Explore popovers should close on escape #19902
feat: Explore popovers should close on escape #19902
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19902 +/- ##
=======================================
Coverage 66.54% 66.54%
=======================================
Files 1714 1714
Lines 65102 65118 +16
Branches 6725 6729 +4
=======================================
+ Hits 43321 43335 +14
- Misses 20069 20070 +1
- Partials 1712 1713 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
Show resolved
Hide resolved
...props | ||
}) => { | ||
const triggerElementRef = useRef<HTMLElement>(); | ||
|
||
const [visible, setVisible] = useState( |
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.
Totally, I don't think that introducing visible
state here is a good idea.
We can control visibleProps
.
Currently you are mixing visible
state and visible
props to control visible, but it is not a good idea.
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.
We need the visible state locally to control the popover in this component.
The "visible" prop can be undefined, (leaving the component in an uncontrolled manner), that's why we need to take over the control, and can't rely on simple calling the onVisibleChange callback
superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx
Show resolved
Hide resolved
4431013
to
2fd9355
Compare
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.
LGTM
🏷️ preset:2022.19 |
* feat: Explore popovers should close on escape * review comments (cherry picked from commit dbc653d)
* feat: Explore popovers should close on escape * review comments
SUMMARY
This PR enables the popovers in the explore to be closed when hitting the escape key.
Also, improves the locking of the explore scroll, so that the popovers are pinned to the triggering div until they're closed.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No way to visually show the difference, so this is the actual result:
new.mov
TESTING INSTRUCTIONS
In the explore:
ADDITIONAL INFORMATION