Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Logs Explorer] Change the default link for "Discover" in the serverless nav #173420

Merged
merged 10 commits into from
Jan 3, 2024
Merged

[Logs Explorer] Change the default link for "Discover" in the serverless nav #173420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fb1459e
Change the default link for "Discover" in the serverless nav
weltenwort Dec 14, 2023
aff3514
Merge remote-tracking branch 'upstream/main' into obs-change-default-…
weltenwort Dec 18, 2023
932e1dc
Fix serverless navigation tests
weltenwort Dec 18, 2023
fa58c8b
Attempt to fix the Discover breadcrumbs
weltenwort Dec 18, 2023
8268407
Merge remote-tracking branch 'upstream/main' into obs-change-default-…
weltenwort Dec 18, 2023
45d5b90
Merge remote-tracking branch 'upstream/main' into obs-change-default-…
weltenwort Dec 20, 2023
ba7c7be
Skip Discover side nav test on serverless-oblt
weltenwort Dec 20, 2023
eeeadc9
Merge remote-tracking branch 'upstream/main' into obs-change-default-…
weltenwort Jan 2, 2024
2598f0a
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Jan 2, 2024
500e0c1
Remove superfluous `sideNavStatus` entries
weltenwort Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion NOTICE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Kibana source code with Kibana X-Pack source code
Copyright 2012-2023 Elasticsearch B.V.
Copyright 2012-2024 Elasticsearch B.V.

---
Pretty handling of logarithmic axes.
Expand Down
15 changes: 11 additions & 4 deletions x-pack/plugins/serverless_observability/public/components/side_navigation/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,20 @@ const navigationTree: NavigationTreeDefinition = {
title: i18n.translate('xpack.serverlessObservability.nav.discover', {
defaultMessage: 'Discover',
}),
link: 'discover',
link: 'observability-log-explorer',
// prevent this entry from ever becoming active, effectively falling through to the obs-log-explorer child
getIsActive: () => false,
Copy link
Contributor

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?

Copy link
Member Author

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 tab
  • Discover, which is needed so Discover shows up in the breadcrumbs and the entry is active when in Discover
  • Logs Explorer, which is needed so the breadcrumbs are Discover / 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.

Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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

// avoid duplicate "Discover" breadcrumbs
breadcrumbStatus: 'hidden',
renderAs: 'item',
children: [
{
// This is to show "observability-log-explorer" breadcrumbs when navigating from "discover" to "log explorer"
link: 'observability-log-explorer',
sideNavStatus: 'hidden',
link: 'discover',
children: [
{
link: 'observability-log-explorer',
},
],
},
],
},
Expand Down
4 changes: 4 additions & 0 deletions x-pack/test_serverless/functional/test_suites/common/discover/group1/_url_state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
});

describe('Side nav', function () {
this.tags([
'skipSvlOblt', // the "Discover" side nav entry does something different in oblt
]);

it('should sync Lens global state to Discover sidebar link and carry over the state when navigating to Discover', async () => {
await PageObjects.common.navigateToApp('discover');
await PageObjects.common.navigateToApp('lens');
Expand Down
14 changes: 9 additions & 5 deletions x-pack/test_serverless/functional/test_suites/observability/navigation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,15 @@ export default function ({ getPageObject, getService }: FtrProviderContext) {
});
await svlCommonNavigation.sidenav.expectSectionClosed('project_settings_project_nav');

// navigate to discover
await svlCommonNavigation.sidenav.clickLink({ deepLinkId: 'discover' });
await svlCommonNavigation.sidenav.expectLinkActive({ deepLinkId: 'discover' });
await svlCommonNavigation.breadcrumbs.expectBreadcrumbExists({ deepLinkId: 'discover' });
expect(await browser.getCurrentUrl()).contain('/app/discover');
// navigate to the logs explorer tab by default
await svlCommonNavigation.sidenav.clickLink({ deepLinkId: 'observability-log-explorer' });
await svlCommonNavigation.sidenav.expectLinkActive({
deepLinkId: 'observability-log-explorer',
});
await svlCommonNavigation.breadcrumbs.expectBreadcrumbExists({
deepLinkId: 'observability-log-explorer',
});
expect(await browser.getCurrentUrl()).contain('/app/observability-log-explorer');

// check the aiops subsection
await svlCommonNavigation.sidenav.openSection('observability_project_nav.aiops'); // open ai ops subsection
Expand Down
Loading