-
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
[Logs Explorer][Discover] Move Logs Overview into Discover codebase #180262
[Logs Explorer][Discover] Move Logs Overview into Discover codebase #180262
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…nyghiani/kibana into refactor/push-flyout-down-to-discover
The reason behind the huge move of async bundle chunks from logs explorer into unified doc viewer is due to the ecs package that loads the fields descriptions. |
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
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.
Thanks for creating the smoke tests. And if it works out, I like the use of the flex grid!
I'm somehow struggling to deploy this to a cloud instance, so I wasn't able to check it visually yet.
I've seen several places where you removed intermediate <EuiFlexItem>
s inside of <EuiFlexGroup>
s. Was that intentional? Why do we need the groups in the first place if they don't contain flex items?
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.
The code changes look good and it worked well when testing! I left a few questions, but otherwise this looks good on my end.
packages/kbn-unified-doc-viewer/src/services/doc_views_registry.ts
Outdated
Show resolved
Hide resolved
...er/public/components/doc_viewer_logs_overview/sub_components/highlight_field_description.tsx
Show resolved
Hide resolved
cc. @weltenwort
I recently had an interesting discussion with the EUI team on an issue I created, and an off-topic comment (the first half of the comment is what matters) directly touched on the usage of EuiFlexGroup + EuiFlexItem. The EuiFlexGroup + EuiFlexItem combo was in place to achieve the
|
…y.ts Co-authored-by: Davis McPhee <[email protected]>
Thanks for teaching me about that new flexbox gap semantic. It makes sense to avoid unnecessary |
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 really like the thought of relying on flex grid to perform the column calculations for us 👍
During testing I found that it behaves differently from the previous implementation:
- It switches to two columns earlier when the screen is small
- It switches to more than three columns when the screen is large, while three was the upper limit previously
I think the new behavior doesn't feel any worse than the previous one. Just wanted to point this out in case there was an important reason for the previous behavior. Maybe @isaclfreire can double-check?
...plugins/observability_solution/logs_explorer/public/customizations/custom_flyout_content.tsx
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.
Thanks for addressing my comments and explaining the EuiFlexGroup
situation! In that case great work on this and it LGTM 👍
@weltenwort @isaclfreire I agree with Felix as it makes easier to tweak and maintain the columns layout using native CSS. auto-fill auto-fit @isaclfreire do you have any preference on the above options? Update: I turned it to auto-fill as IMO looks better aligned, but I'll wait on your check. |
@weltenwort @tonyghiani agreed that autofill looks way more tidy! all looking great btw, thanks :) |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk 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: |
📓 Summary
Closes #180024
This work aims to move the "Logs Overview" doc view created from the logs-explorer app into the unified-doc-viewer plugin, creating and registering this as a preset along the table and source doc views.
To keep control of whether this doc view should be displayed or not, an
enabled
configuration flag is supported for every doc view and will be used to determine whether a doc view should load or not in the view.This
enabled
flag can be programmatically enabled/disabled bydocView.id
using the 2 new methods added to theDocViewsRegistry
, theenableById
anddisableById
ones.The customization extension point does not register the content of the tab, but is limited to enable/disable a preset overview tab.
To allow this change, some shared utilities between the flyout logic and the smart fields feature have been copied into the
@kbn/discover-utils
package. The utils will still live in the logs_explorer plugin and are used by the smart fields feature until this is migrated too into Discover.💡 Reviewer hints
Although it seems a large PR, most of the changes are on moved files from logs-explorer into unified-doc-viewer, which is logic already reviewed. With these changes, there will be a follow-up PR to optimize the shared parts with the other doc views.
🚶 Next steps
To keep the scope of this PR the smallest possible, here are some changes I'll work out in upcoming PRs built on top of this one.
tonyghiani#1