-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
fix(explore): adhoc metrics popover resets label after hovering outside #16196
Conversation
/testenv up |
@kgabryje Ephemeral environment spinning up at http://34.217.60.12:8080. Credentials are |
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.
Code LGTM with minor nit. I tried to make this break in every possible way I could but wasn't able to come up with any bugs/regressions. Extra kudos for net negative LOC PR!
superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx
Outdated
Show resolved
Hide resolved
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 pulled the branch and tried to reproduce the bug that I had become very acquainted with last night and could not reproduce. Tried with flags ENABLE_EXPLORE_DRAG_AND_DROP
and UX_BETA
on and off and couldn't repro the bug or didn't find any other issues.
Codecov Report
@@ Coverage Diff @@
## master #16196 +/- ##
==========================================
- Coverage 76.83% 76.73% -0.10%
==========================================
Files 996 996
Lines 53060 52999 -61
Branches 6766 6738 -28
==========================================
- Hits 40766 40670 -96
- Misses 12066 12100 +34
- Partials 228 229 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Ephemeral environment shutdown and build artifacts deleted. |
🏷 2021.31 |
…de (apache#16196) * fix(explore): adhoc metrics popover resets label after hovering outside * Remove irrelevant tests and skip rest * Use ensureIsArray (cherry picked from commit ccfc95f)
…de (apache#16196) * fix(explore): adhoc metrics popover resets label after hovering outside * Remove irrelevant tests and skip rest * Use ensureIsArray
…de (apache#16196) * fix(explore): adhoc metrics popover resets label after hovering outside * Remove irrelevant tests and skip rest * Use ensureIsArray (cherry picked from commit ccfc95f)
…de (apache#16196) * fix(explore): adhoc metrics popover resets label after hovering outside * Remove irrelevant tests and skip rest * Use ensureIsArray
…de (apache#16196) * fix(explore): adhoc metrics popover resets label after hovering outside * Remove irrelevant tests and skip rest * Use ensureIsArray (cherry picked from commit ccfc95f)
…de (apache#16196) * fix(explore): adhoc metrics popover resets label after hovering outside * Remove irrelevant tests and skip rest * Use ensureIsArray (cherry picked from commit 1c93bed)
SUMMARY
When user opened "Add new metric" popover and selected a column and/or aggregate, metric title is automatically set to "column(aggregate)". However, when user hovers with their mouse outside of the popover before saving, the title gets reset. Then, after saving, the label appears to be correct, but an empty string gets passed to executed query as a label, causing seemingly unrelated errors.
This PR converts
MetricsControl
component from class to functional and uses memoization to prevent resetting the title.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: see linked issue and video in the comments
After:
https://user-images.githubusercontent.com/15073128/129045681-e33f2ab4-4aac-4d95-b928-6f5c3705e13f.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CC @graceguo-supercat @junlincc