-
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] Split saved searches out of discover plugin #138388
Conversation
… into split-out-visualize-app
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
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.
limits.yml
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, testing by merging with the lens in Discover POC, cyclic dependency resolved. thx for taking care of the heavy lifting 👍
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.
Tested locally 👍
Left only a tiny nitpick comment.
import { SavedSearch } from '@kbn/saved-search-plugin/public'; | ||
import type { SortOrder } from '@kbn/saved-search-plugin/public'; |
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.
nit: can these be merged?
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.
that's what this PR will do ... once the merge conflicts are resolved: #137488
Joe's PR went first, because I doubt he already has grey hair ... while one gray hair more for me, doesn't matter 👴
This PR splits shared saved search logic (used by discover and visualize) out of the discover plugin into a separate plugin. This will allow to implement the histogram in Discover using a Lens embeddable in a later step (otherwise it would have introduced a cyclic dependency).
This PR is mostly moving over the saved search service - react hook and conflict component are left within Discover because they are not used anywhere else.