-
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
[Logs Explorer] Change the default link for "Discover" in the serverless nav #173420
[Logs Explorer] Change the default link for "Discover" in the serverless nav #173420
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@davismcphee I'd appreciate your help with something: There is this test at https://github.com/weltenwort/kibana/blob/8268407ed15c64ba2f4125ca6519ccff4fc2236a/x-pack/test_serverless/functional/test_suites/common/discover/group1/_url_state.ts#L80, which explicitly tests the persistence of some Discover state in the side nav link. Since this PR changes the default target of the side nav entry, the test doesn't work anymore. Would you say it can be dropped completely or does it cover anything about Discover's state that you'd like to preserve and move elsewhere? |
cc @elastic/kibana-data-discovery ☝️ 😇 |
It's a test to check that filters (including pinned filters) are carried over correctly. Can it also work for Logs Explorer link? Would be great to keep the test for ES and Security projects at least. |
No, the Logs Explorer doesn't persist its state globally that way. If we're going to implement something similar we'd probably use the LocalStorage approach as it is less entangled with the chrome service. |
I didn't even know there are tests that are executed for every serverless project type. Do you know how to limit them to a sub-set? |
I think we would need to move the tests from |
/ci |
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
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.
Data Discovery changes LGTM 👍
link: 'discover', | ||
link: 'observability-log-explorer', | ||
// prevent this entry from ever becoming active, effectively falling through to the obs-log-explorer child | ||
getIsActive: () => false, |
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.
Don't we want ever to mark this link as active?
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 menu hierarchy here is:
Logs Explorer
, which is never active and hides its breadcrumb, but needed so the Logs Explorer is the default tabDiscover
, which is needed soDiscover
shows up in the breadcrumbs and the entry is active when in DiscoverLogs Explorer
, which is needed so the breadcrumbs areDiscover
/Logs Explorer
and the entry is also active when in the Logs Explorer
It's the only way I found to get the serverless nav and breadcrumbs to behave that way. If you know of any other approach to achieve the same thing I'd gladly adopt it.
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 just checked and indeed the way multiple active node paths (here you have 2 pointing to the observability-log-explorer
link) are declared forces you to do this. I'll open a PR to fix it in the serverless nav so you don't need to hack it here.
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 would be great. Would you prefer us to wait for your change or shall I align with it in a follow-up?
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.
Sorry just reading now. All good in opened my PR after you merged 👍
#174184
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.
@maryam-saeidi The goal is that the Discover tab is still there, but the Logs Explorer tab is the default. Are you seeing a different behavior in your tests? |
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 👍 I'll open a PR for my comment about getIsActive()
link: 'discover', | ||
link: 'observability-log-explorer', | ||
// prevent this entry from ever becoming active, effectively falling through to the obs-log-explorer child | ||
getIsActive: () => false, |
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 just checked and indeed the way multiple active node paths (here you have 2 pointing to the observability-log-explorer
link) are declared forces you to do this. I'll open a PR to fix it in the serverless nav so you don't need to hack it here.
x-pack/plugins/serverless_observability/public/components/side_navigation/index.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: cc @weltenwort |
run elasticsearch-ci/docs |
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.
Worked as expected 👍🏻
* main: (4129 commits) [Logs Explorer] Change the default link for "Discover" in the serverless nav (#173420) [Fleet] fix unhandled error in agent details when components are missing (#174152) [Obs UX] Unskip transaction duration alerts test (#174069) [Fleet] Fix keep policies up to date after package install (#174093) [Profiling] Stack traces embeddable (#173905) [main] Sync bundled packages with Package Storage (#174115) [SLO Form] Refactor to use kibana data view component (#173513) [Obs UX] Unskip APM Service Inventory Journey (#173510) [Obs UX] Unskip preview_chart_error_count test (#173624) [api-docs] 2024-01-03 Daily api_docs build (#174142) Update babel runtime helpers (#171745) Handle content stream errors in report pre-deletion (#173792) [Cloud Posture] [Quick wins] Enable edit DataView on the Misconfigurations Findings Table (#173870) [ftr] abort retry on invalid webdriver session (#174092) Upgrade openai to 4.24.1 (#173934) chore(NA): bump node into v20 (#173461) [Security Solution][Endpoint] Fix index name pattern in SentinelOne dev. script (#174105) fix versions.json [Obs AI Assistant] Add guardrails (#174060) [ML] Transforms: Refactor validators and add unit tests. (#173736) ...
📓 Summary
This changes the navigation target of the "Discover" sidebar menu in the observability serverless project type to be the Logs Explorer.
cc @ruflin