-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: Issues with filters and metrics popovers #11578
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11578 +/- ##
==========================================
- Coverage 59.59% 54.96% -4.64%
==========================================
Files 832 412 -420
Lines 40482 14281 -26201
Branches 3666 3662 -4
==========================================
- Hits 24126 7849 -16277
+ Misses 16187 6263 -9924
Partials 169 169
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Fixes #11546 |
@@ -50,7 +50,11 @@ const defaultProps = { | |||
columns: [], | |||
}; | |||
|
|||
const startingWidth = 300; | |||
const ResizeIcon = styled.i` |
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 see this is happening a couple times in this PR, but I can't think of an easy way to DRY this up that would be worth doing if we come through and start replacing all the font-awesome icons in another pass anyway. Only mentioning it in case you have an idea of how to simply consolidate the two ResizeIcon
instances... but it's probably not worth worrying about. So now I'm just rambling...
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 felt bad when repeating that code, but I couldn't see an easy way not to 🙁 In general, I think it might be worth it to refactor Metric and Filter popovers, because they are rather similar, with only tabs content differing.
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.
Made a rambling note, but I think this LGTM :)
created new issue.. #11598 |
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.
Thank
* Fix bugs in AdhocFilterEditPopover * Fix bugs in AdhocMetricEditPopover * Remove handleMultiComparatorInputHeightChange function * Fix tests
SUMMARY
Fixes issues described in #11546.
There was also another bug - select menu wasn't fully visible when it overflew the popover (which is almost always) - see the first gif.
The filters popover was jumpy and it was related to
handleMultiComparatorInputHeightChange
function inAdhocFilterEditPopoverSimpleTabContent.jsx
file. I removed it, however if it was actually necessary for something I'd appreciate a feedback (@ktmud the last changes to that function was done by you - can you take a look?).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TEST PLAN
ADDITIONAL INFORMATION