-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Discover] Update unified histogram to use Lens #143117
[Discover] Update unified histogram to use Lens #143117
Conversation
Documentation preview: |
17dd348
to
a08d947
Compare
477f1d4
to
838445c
Compare
…ns-embeddable [Discover] Fix functional tests
@davismcphee I brought this question to the EUI team's weekly meetup today. We recommend using the titles that appear on hover automatically, and adding the prop |
@1Copenut Thanks for looking into this. Unless I'm misunderstanding something, it seems like this suggestion might not be possible, at least the text wrapping part of it. The component we're using here is |
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.
Awesome work! Happy to see the new chart + breakdown capabilities 👍
if (storage) { | ||
services = { ...services, storage }; | ||
} | ||
(services.data.query.queryString.getDefaultQuery as jest.Mock).mockReturnValue({ |
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 could add it inside createDiscoverServicesMock
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 agree with this, but I tried to update quickly and it broke some unrelated tests that were expecting getDefaultQuery
to return undefined
. I think it's a good cleanup task though, just would prefer to save for another PR.
@@ -124,7 +124,7 @@ describe('checking migration metadata changes on all registered SO types', () => | |||
"osquery-saved-query": "7b213b4b7a3e59350e99c50e8df9948662ed493a", | |||
"query": "4640ef356321500a678869f24117b7091a911cb6", | |||
"sample-data-telemetry": "8b10336d9efae6f3d5593c4cc89fb4abcdf84e04", | |||
"search": "d26771bcf7cd271162aab3a610b75249631ef6b1", | |||
"search": "b212646cedccdfb0f8504426f72d1e087c4bab7c", |
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.
Curious how this change was introduced?
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.
These are hashes of saved object mappings to alert the core team when the mappings are updated. The hash got updated once when we added breakdownField
to the PR, but it got updated again when I merged main recently since we also added usesAdHocDataView
. Basically I just needed to update it again to match the latest hash.
The EUI team could look at adding a prop to define beginning/middle/end truncation. I'll open a feature issue for it and link it here. This feels like the best alternative we can offer given the boundaries of same height for virtualization. |
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.
ML edits LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @davismcphee |
Summary
This PR updates Discover and Unified Histogram to use Lens for the histogram chart, and removes Discover as a Lens dependency:
Note that there are some additional changes to make in followup PRs before this will be ready for solutions adoption:
A la carte deployment: https://davismcphee-pr-143117-enhancement-unified-histogram-lens.kbndev.co.
Flaky test runs:
Resolves #143108.
Resolves #142390.
Checklist
Documentation was added for features that require explanation or tutorialsAny UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listFor maintainers