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.

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

[navigation]Refactor: flatten left nav in Analytics(all) use case #8332

Merged
merged 41 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
67a0a11
feat: move the logic to construct navLinks in all use case to nav_gro…
SuZhou-Joe Sep 25, 2024
2532dec
feat: change category
SuZhou-Joe Sep 25, 2024
86dae40
feat: register search overview to all use case
SuZhou-Joe Sep 25, 2024
cc1fca7
Changeset file for PR #8332 created/updated
opensearch-changeset-bot[bot] Sep 25, 2024
d82c4f0
feat: fix bootstrap
SuZhou-Joe Sep 25, 2024
4a88a34
feat: show icon in category to expand or collapse
SuZhou-Joe Sep 26, 2024
b39d9d7
feat: merge fix/fit-finish
SuZhou-Joe Sep 26, 2024
6ea23c5
feat: finish whole refactor
SuZhou-Joe Sep 26, 2024
6bf0d6b
feat: update
SuZhou-Joe Sep 26, 2024
3578916
feat: update
SuZhou-Joe Sep 26, 2024
4546d02
feat: style update
SuZhou-Joe Sep 27, 2024
9e66bc5
fix: unit test
SuZhou-Joe Sep 29, 2024
dc562e4
fix: unit tets
SuZhou-Joe Sep 29, 2024
be1ee80
Changeset file for PR #8332 created/updated
opensearch-changeset-bot[bot] Sep 29, 2024
82334d4
feat: remove useless code
SuZhou-Joe Sep 29, 2024
13d4ec8
feat: update snapshot
SuZhou-Joe Sep 29, 2024
1916078
fix: i18n
SuZhou-Joe Sep 30, 2024
4a9b206
feat: update
SuZhou-Joe Sep 30, 2024
1a12067
feat: update
SuZhou-Joe Sep 30, 2024
8296d76
feat: update
SuZhou-Joe Sep 30, 2024
3450737
feat: update style
SuZhou-Joe Sep 30, 2024
4a4eaad
feat: update style
SuZhou-Joe Sep 30, 2024
061108d
feat: update
SuZhou-Joe Sep 30, 2024
acf325d
feat: update
SuZhou-Joe Oct 1, 2024
da8cc6a
feat: update
SuZhou-Joe Oct 1, 2024
3d047e9
feat: merge
SuZhou-Joe Oct 2, 2024
71ea8f0
feat: remove useless scss
SuZhou-Joe Oct 2, 2024
988ca6d
feat: update
SuZhou-Joe Oct 2, 2024
20365fa
Merge branch 'main' into refactor/all-use-case
SuZhou-Joe Oct 2, 2024
cb44610
feat: change icon color to text and update the style a little bit
SuZhou-Joe Oct 2, 2024
89ba989
feat: update
SuZhou-Joe Oct 2, 2024
ea25014
feat: update
SuZhou-Joe Oct 2, 2024
7da0bfe
feat: update naming
SuZhou-Joe Oct 2, 2024
faded17
feat: optimize code
SuZhou-Joe Oct 2, 2024
bfc1f60
feat: optimize code
SuZhou-Joe Oct 2, 2024
fde8c9b
feat: optimize code
SuZhou-Joe Oct 2, 2024
9f7d497
fix: warning
SuZhou-Joe Oct 2, 2024
0e083b1
fix: bootstrap
SuZhou-Joe Oct 2, 2024
da5e4fe
feat: update
SuZhou-Joe Oct 2, 2024
b6f4b16
fix: unit test
SuZhou-Joe Oct 2, 2024
a10cb84
feat: optimize code based on comments
SuZhou-Joe Oct 3, 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
3 changes: 3 additions & 0 deletions changelogs/fragments/8332.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
feat:
- [navigation] flatten left nav in Analytics(all) use case ([#8332](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8332))
- [navigation] Adjust the appearances of the left navigation menu and the landing page ([#8332](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8332))
103 changes: 60 additions & 43 deletions src/core/public/chrome/nav_group/nav_group_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,66 @@ describe('ChromeNavGroupService#start()', () => {
expect(groupsMap[mockedGroupBar.id].navLinks.length).toEqual(1);
});

it('should populate links with custom category if the nav link is inside second level but no entry in all use case', async () => {
const chromeNavGroupService = new ChromeNavGroupService();
const uiSettings = uiSettingsServiceMock.createSetupContract();
const chromeNavGroupServiceSetup = chromeNavGroupService.setup({ uiSettings });

chromeNavGroupServiceSetup.addNavLinksToGroup(DEFAULT_NAV_GROUPS.all, [
{
id: 'foo',
},
]);
chromeNavGroupServiceSetup.addNavLinksToGroup(DEFAULT_NAV_GROUPS.essentials, [
{
id: 'bar',
title: 'bar',
},
{
id: 'foo',
title: 'foo',
},
]);
const navLinkServiceStart = mockedNavLink.start({
http: mockedHttpService,
application: mockedApplicationService,
});
navLinkServiceStart.getNavLinks$ = jest.fn().mockReturnValue(
new Rx.BehaviorSubject([
{
id: 'foo',
},
{
id: 'bar',
},
{
id: 'customized_app',
},
])
);
const chromeStart = await chromeNavGroupService.start({
navLinks: navLinkServiceStart,
application: mockedApplicationService,
breadcrumbsEnricher$: new Rx.BehaviorSubject<ChromeBreadcrumbEnricher | undefined>(undefined),
workspaces: workspacesServiceMock.createStartContract(),
});
const groupsMap = await chromeStart.getNavGroupsMap$().pipe(first()).toPromise();
expect(groupsMap[ALL_USE_CASE_ID].navLinks).toEqual([
{
id: 'foo',
},
{
id: 'bar',
title: 'bar',
category: { id: 'custom', label: 'Custom', order: 8500 },
},
{
id: 'customized_app',
category: { id: 'custom', label: 'Custom', order: 8500 },
},
]);
});

it('should return navGroupEnabled from ui settings', async () => {
const chromeNavGroupService = new ChromeNavGroupService();
const uiSettings = uiSettingsServiceMock.createSetupContract();
Expand Down Expand Up @@ -381,49 +441,6 @@ describe('ChromeNavGroupService#start()', () => {
expect(currentNavGroup?.title).toEqual('barGroupTitle');
});

it('should be able to find the right nav group when visible nav group is all', async () => {
const uiSettings = uiSettingsServiceMock.createSetupContract();
const navGroupEnabled$ = new Rx.BehaviorSubject(true);
uiSettings.get$.mockImplementation(() => navGroupEnabled$);

const chromeNavGroupService = new ChromeNavGroupService();
const chromeNavGroupServiceSetup = chromeNavGroupService.setup({ uiSettings });

chromeNavGroupServiceSetup.addNavLinksToGroup(
{
id: ALL_USE_CASE_ID,
title: 'fooGroupTitle',
description: 'foo description',
},
[mockedNavLinkFoo]
);

chromeNavGroupServiceSetup.addNavLinksToGroup(
{
id: 'bar-group',
title: 'barGroupTitle',
description: 'bar description',
status: NavGroupStatus.Hidden,
},
[mockedNavLinkFoo, mockedNavLinkBar]
);

const chromeNavGroupServiceStart = await chromeNavGroupService.start({
navLinks: mockedNavLinkService,
application: mockedApplicationService,
breadcrumbsEnricher$: new Rx.BehaviorSubject<ChromeBreadcrumbEnricher | undefined>(undefined),
workspaces: workspacesServiceMock.createStartContract(),
});
mockedApplicationService.navigateToApp(mockedNavLinkBar.id);

const currentNavGroup = await chromeNavGroupServiceStart
.getCurrentNavGroup$()
.pipe(first())
.toPromise();

expect(currentNavGroup?.id).toEqual('bar-group');
});

it('should be able to find the right nav group when visible nav group length is 1 and is not all nav group', async () => {
const uiSettings = uiSettingsServiceMock.createSetupContract();
const navGroupEnabled$ = new Rx.BehaviorSubject(true);
Expand Down
120 changes: 103 additions & 17 deletions src/core/public/chrome/nav_group/nav_group_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
ChromeNavLink,
WorkspacesStart,
} from 'opensearch-dashboards/public';
import { i18n } from '@osd/i18n';
import { map, switchMap, takeUntil } from 'rxjs/operators';
import { IUiSettingsClient } from '../../ui_settings';
import {
Expand All @@ -22,7 +23,7 @@
import { InternalApplicationStart } from '../../application';
import { NavGroupStatus, NavGroupType } from '../../../../core/types';
import { ChromeBreadcrumb, ChromeBreadcrumbEnricher } from '../chrome_service';
import { ALL_USE_CASE_ID } from '../../../utils';
import { ALL_USE_CASE_ID, DEFAULT_APP_CATEGORIES } from '../../../utils';

export const CURRENT_NAV_GROUP_ID = 'core.chrome.currentNavGroupId';

Expand Down Expand Up @@ -74,6 +75,14 @@
setCurrentNavGroup: (navGroupId: string | undefined) => void;
}

// Custom category is used for those features not belong to any of use cases in all use case.
// and the custom category should always sit after manage category
const customCategory: AppCategory = {
id: 'custom',
label: i18n.translate('core.ui.customNavList.label', { defaultMessage: 'Custom' }),
order: (DEFAULT_APP_CATEGORIES.manage.order || 0) + 500,
};

/** @internal */
export class ChromeNavGroupService {
private readonly navGroupsMap$ = new BehaviorSubject<Record<string, NavGroupItemInMap>>({});
Expand Down Expand Up @@ -114,12 +123,85 @@
}

private sortNavGroupNavLinks(
navGroup: NavGroupItemInMap,
navLinks: NavGroupItemInMap['navLinks'],
allValidNavLinks: Array<Readonly<ChromeNavLink>>
) {
return getSortedNavLinks(
fulfillRegistrationLinksToChromeNavLinks(navGroup.navLinks, allValidNavLinks)
);
return getSortedNavLinks(fulfillRegistrationLinksToChromeNavLinks(navLinks, allValidNavLinks));
}

private getNavLinksForAllUseCase(
navGroupsMap: Record<string, NavGroupItemInMap>,
navLinks: Array<Readonly<ChromeNavLink>>
) {
// Note: we need to use a new pointer when `assign navGroupsMap[ALL_USE_CASE_ID]?.navLinks`
// because we will mutate the array directly in the following code.
const navLinksResult: ChromeRegistrationNavLink[] = [
...(navGroupsMap[ALL_USE_CASE_ID]?.navLinks || []),
];

// Append all the links that do not have use case info to keep backward compatible
const linkIdsWithNavGroupInfo = Object.values(navGroupsMap).reduce((accumulator, navGroup) => {
if (!navGroup.type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a comment for the if condition 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.

Sure, updated.

// Append use case section into left navigation
const categoryInfo = {
id: navGroup.id,
label: navGroup.title,
order: navGroup.order,
};

const fulfilledLinksOfNavGroup = fulfillRegistrationLinksToChromeNavLinks(
navGroup.navLinks,
navLinks
);

const linksForAllUseCaseWithinNavGroup: ChromeRegistrationNavLink[] = [];

fulfilledLinksOfNavGroup.forEach((navLink) => {
if (!navLink.showInAllNavGroup) {
return;
}

linksForAllUseCaseWithinNavGroup.push({

Check warning on line 164 in src/core/public/chrome/nav_group/nav_group_service.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/chrome/nav_group/nav_group_service.ts#L164

Added line #L164 was not covered by tests
...navLink,
category: categoryInfo,
});
});

navLinksResult.push(...linksForAllUseCaseWithinNavGroup);

if (!linksForAllUseCaseWithinNavGroup.length) {
/**
* Find if there are any links inside a use case but without a `see all` entry.
* If so, append these features into custom category as a fallback
*/
fulfillRegistrationLinksToChromeNavLinks(navGroup.navLinks, navLinks).forEach(
(navLink) => {
// Links that already exists in all use case do not need to reappend
if (navLinksResult.find((navLinkInAll) => navLinkInAll.id === navLink.id)) {
return;
}
navLinksResult.push({
...navLink,
category: customCategory,
});
}
);
}
}

return [...accumulator, ...navGroup.navLinks.map((navLink) => navLink.id)];
}, [] as string[]);
navLinks.forEach((navLink) => {
if (linkIdsWithNavGroupInfo.includes(navLink.id)) {
return;
}
navLinksResult.push({
...navLink,
category: customCategory,
});
});

return navLinksResult;
}

private getSortedNavGroupsMap$() {
Expand All @@ -129,10 +211,20 @@
map(([navGroupsMap, navLinks]) => {
return Object.keys(navGroupsMap).reduce((sortedNavGroupsMap, navGroupId) => {
const navGroup = navGroupsMap[navGroupId];
sortedNavGroupsMap[navGroupId] = {
...navGroup,
navLinks: this.sortNavGroupNavLinks(navGroup, navLinks),
};
if (navGroupId === ALL_USE_CASE_ID) {
sortedNavGroupsMap[navGroupId] = {
...navGroup,
navLinks: this.sortNavGroupNavLinks(
this.getNavLinksForAllUseCase(navGroupsMap, navLinks),
navLinks
),
};
} else {
sortedNavGroupsMap[navGroupId] = {
...navGroup,
navLinks: this.sortNavGroupNavLinks(navGroup.navLinks, navLinks),
};
}
return sortedNavGroupsMap;
}, {} as Record<string, NavGroupItemInMap>);
})
Expand Down Expand Up @@ -270,14 +362,8 @@
});
};
if (visibleUseCases.length === 1) {
if (visibleUseCases[0].id === ALL_USE_CASE_ID) {
// If the only visible use case is all use case
// All the other nav groups will be visible because all use case can visit all of the nav groups.
Object.values(navGroupMap).forEach((navGroup) => mapAppIdToNavGroup(navGroup));
} else {
// It means we are in a workspace, we should only use the visible use cases
visibleUseCases.forEach((navGroup) => mapAppIdToNavGroup(navGroup));
}
// It means we are in a workspace, we should only use the visible use cases
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't quite get this comment, could you elaborate a bit please?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, updated.

visibleUseCases.forEach((navGroup) => mapAppIdToNavGroup(navGroup));
} else {
// Nav group of Hidden status should be filtered out when counting navGroups the currentApp belongs to
Object.values(navGroupMap).forEach((navGroup) => {
Expand Down
Loading
Loading