-
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
[controls] fix timeslider control not filtering dashboard panels #145184
Conversation
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Discover data-test-subj
change LGTM
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!
code review and tested in chrome.
@@ -11,6 +11,7 @@ import React from 'react'; | |||
import ReactDOM from 'react-dom'; | |||
import { compareFilters, COMPARE_ALL_OPTIONS, Filter, uniqFilters } from '@kbn/es-query'; | |||
import { BehaviorSubject, merge, Subject, Subscription } from 'rxjs'; | |||
import _ from 'lodash'; |
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.
Honest question, because I really don't know. Would import { isEqual } from 'lodash';
be any better for compilation or performance?
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 found this article which demonstrates that either method produces the same bundle size. I am not sure how the 2 methods effect compilation speed.
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.
Makes sense. Looks like we also have lodash in @kbn/ui-shared-deps-src. And I've found more context here. tldr; don't import from 'lodash/*
.
#140739 caused a regression with timeslider control where
ControlGroupContainer
was not not firingupdateOutput
onoutput.timeslice
changes.This PR resolves the regression and adds functional tests to ensure interactions with timeslider properly flow to dashboard saved search panel