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-next] fix: redirect to standard index pattern applications while nav group is enabled #7305

2 changes: 2 additions & 0 deletions changelogs/fragments/7305.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- [navigation-next] fix: redirect to standard index pattern applications while nav group is enabled ([#7305](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7305))
47 changes: 0 additions & 47 deletions src/core/public/chrome/nav_group/nav_group_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,53 +311,6 @@ describe('ChromeNavGroupService#start()', () => {
expect(sessionStorageMock.getItem(CURRENT_NAV_GROUP_ID)).toBeFalsy();
expect(currentNavGroup).toBeUndefined();
});

it('should reset current nav group if app not belongs to any nav group', 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: 'foo',
title: 'foo title',
description: 'foo description',
},
[{ id: 'foo-app1' }]
);

const chromeNavGroupServiceStart = await chromeNavGroupService.start({
navLinks: mockedNavLinkService,
application: mockedApplicationService,
});

// set an existing nav group id
chromeNavGroupServiceStart.setCurrentNavGroup('foo');

expect(sessionStorageMock.getItem(CURRENT_NAV_GROUP_ID)).toEqual('foo');

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

expect(currentNavGroup?.id).toEqual('foo');

// navigate to app don't belongs to any nav group
mockedApplicationService.navigateToApp('bar-app');

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

// verify current nav group been reset
expect(currentNavGroup).toBeFalsy();
expect(sessionStorageMock.getItem(CURRENT_NAV_GROUP_ID)).toBeFalsy();
});
});

describe('nav group updater', () => {
Expand Down
12 changes: 0 additions & 12 deletions src/core/public/chrome/nav_group/nav_group_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,18 +212,6 @@ export class ChromeNavGroupService {
}
};

// erase current nav group when switch app don't belongs to any nav group
application.currentAppId$.subscribe((appId) => {
const navGroupMap = this.navGroupsMap$.getValue();
const appIdsWithNavGroup = Object.values(navGroupMap).flatMap(({ navLinks: links }) =>
links.map(({ id }) => id)
);

if (appId && !appIdsWithNavGroup.includes(appId)) {
setCurrentNavGroup(undefined);
}
});

const currentNavGroupSorted$ = combineLatest([
this.getSortedNavGroupsMap$(),
this.currentNavGroup$,
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
border: none !important;

.nav-link-item {
padding: $ouiSize / 4 $ouiSize;
border-radius: $ouiSize;
padding: calc($euiSize / 4) $euiSize;
border-radius: $euiSize;
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes expected? Seems unrelated

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Jul 20, 2024

Choose a reason for hiding this comment

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

It is expected, as @AMoo-Miki suggested me to do so.

box-shadow: none;
margin-bottom: 0;
margin-top: 0;
Expand Down Expand Up @@ -39,11 +39,20 @@
}

.bottom-container {
padding: 0 $ouiSize;
padding: 0 $euiSize;
display: flex;

&.bottom-container-collapsed {
flex-direction: column;
align-items: center;

> * {
margin: $euiSizeS 0;
}
}
}

.nav-controls-padding {
padding: $ouiSize;
padding: $euiSize;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -223,4 +223,24 @@ describe('<CollapsibleNavGroupEnabled />', () => {
fireEvent.click(getByTestId('back'));
expect(getAllByTestId('collapsibleNavAppLink-link-in-analytics').length).toEqual(2);
});

it('should hide left navigation when in home page', async () => {
const props = mockProps({
navGroupsMap: {
[DEFAULT_NAV_GROUPS.analytics.id]: {
...DEFAULT_NAV_GROUPS.analytics,
navLinks: [
{
id: 'link-in-analytics',
title: 'link-in-analytics',
showInAllNavGroup: true,
},
],
},
},
});
props.appId$ = new BehaviorSubject('home');
const { container } = render(<CollapsibleNavGroupEnabled {...props} isNavOpen />);
expect(container).toMatchSnapshot();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import * as Rx from 'rxjs';
import classNames from 'classnames';
import { ChromeNavControl, ChromeNavLink } from '../..';
import { NavGroupStatus } from '../../../../types';
import { AppCategory, NavGroupStatus } from '../../../../types';
import { InternalApplicationStart } from '../../../application/types';
import { HttpStart } from '../../../http';
import { OnIsLockedUpdate } from './';
Expand Down Expand Up @@ -164,6 +164,13 @@
);
}

// custom category is used for those features not belong to any of use cases in all use case.
const customCategory: AppCategory = {
id: 'custom',
label: i18n.translate('core.ui.customNavList.label', { defaultMessage: 'Custom' }),
order: Number.MAX_SAFE_INTEGER,
};

export function CollapsibleNavGroupEnabled({
basePath,
id,
Expand All @@ -183,29 +190,6 @@
const navGroupsMap = useObservable(observables.navGroupsMap$, {});
const currentNavGroup = useObservable(observables.currentNavGroup$, undefined);

const onGroupClick = (
e: React.MouseEvent<HTMLButtonElement, MouseEvent>,
group: NavGroupItemInMap
) => {
const fulfilledLinks = fulfillRegistrationLinksToChromeNavLinks(
navGroupsMap[group.id]?.navLinks,
navLinks
);
setCurrentNavGroup(group.id);

// the `navGroupsMap[group.id]?.navLinks` has already been sorted
const firstLink = fulfilledLinks[0];
if (firstLink) {
const propsForEui = createEuiListItem({
link: firstLink,
appId,
dataTestSubj: 'collapsibleNavAppLink',
navigateToApp,
});
propsForEui.onClick(e);
}
};

const navLinksForRender: ChromeNavLink[] = useMemo(() => {
if (currentNavGroup) {
return fulfillRegistrationLinksToChromeNavLinks(
Expand Down Expand Up @@ -234,7 +218,10 @@
navLinks
.filter((link) => !linkIdsWithUseGroupInfo.includes(link.id))
.forEach((navLink) => {
navLinksForAll.push(navLink);
navLinksForAll.push({

Check warning on line 221 in src/core/public/chrome/ui/header/collapsible_nav_group_enabled.tsx

View check run for this annotation

Codecov / codecov/patch

src/core/public/chrome/ui/header/collapsible_nav_group_enabled.tsx#L221

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

// Append all the links registered to all use case
Expand Down Expand Up @@ -281,6 +268,33 @@
return 270;
}, [isNavOpen]);

if (appId === 'home') {
return null;
}

const onGroupClick = (
e: React.MouseEvent<HTMLButtonElement, MouseEvent>,
group: NavGroupItemInMap
) => {
const fulfilledLinks = fulfillRegistrationLinksToChromeNavLinks(
navGroupsMap[group.id]?.navLinks,
navLinks
);
setCurrentNavGroup(group.id);

// the `navGroupsMap[group.id]?.navLinks` has already been sorted
const firstLink = fulfilledLinks[0];
if (firstLink) {
const propsForEui = createEuiListItem({
link: firstLink,
appId,
dataTestSubj: 'collapsibleNavAppLink',
navigateToApp,
});
propsForEui.onClick(e);
}
};

return (
<EuiFlyout
data-test-subj="collapsibleNav"
Expand Down Expand Up @@ -335,7 +349,12 @@
</EuiPanel>
</div>
<EuiHorizontalRule margin="none" />
<div className="bottom-container" style={{ flexDirection: isNavOpen ? 'row' : 'column' }}>
<div
className={classNames({
'bottom-container': true,
'bottom-container-collapsed': !isNavOpen,
})}
>
<HeaderNavControls
navControls$={observables.navControlsLeftBottom$}
className={classNames({ 'nav-controls-padding': isNavOpen })}
Expand Down
9 changes: 8 additions & 1 deletion src/core/utils/default_app_categories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,13 @@ export const DEFAULT_APP_CATEGORIES: Record<string, AppCategory> = Object.freeze
label: i18n.translate('core.ui.manageNav.label', {
defaultMessage: 'Manage',
}),
order: 7000,
order: 8000,
},
manageData: {
id: 'manageData',
label: i18n.translate('core.ui.manageDataNav.label', {
defaultMessage: 'Manage data',
}),
order: 1000,
},
});
4 changes: 2 additions & 2 deletions src/plugins/dashboard/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -456,14 +456,14 @@ export class DashboardPlugin
core.chrome.navGroup.addNavLinksToGroup(DEFAULT_NAV_GROUPS.observability, [
{
id: app.id,
order: 300,
order: 400,
category: undefined,
},
]);
core.chrome.navGroup.addNavLinksToGroup(DEFAULT_NAV_GROUPS['security-analytics'], [
{
id: app.id,
order: 300,
order: 400,
category: undefined,
},
]);
Expand Down
15 changes: 10 additions & 5 deletions src/plugins/data_source_management/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,8 @@ export class DataSourceManagementPlugin
core.chrome.navGroup.addNavLinksToGroup(DEFAULT_NAV_GROUPS.dataAdministration, [
{
id: DSM_APP_ID_FOR_STANDARD_APPLICATION,
category: {
id: DSM_APP_ID_FOR_STANDARD_APPLICATION,
label: PLUGIN_NAME,
order: 200,
},
category: DEFAULT_APP_CATEGORIES.manageData,
order: 100,
},
]);

Expand Down Expand Up @@ -186,6 +183,14 @@ export class DataSourceManagementPlugin
},
]);

core.chrome.navGroup.addNavLinksToGroup(DEFAULT_NAV_GROUPS.all, [
{
id: DSM_APP_ID_FOR_STANDARD_APPLICATION,
category: DEFAULT_APP_CATEGORIES.manage,
order: 100,
},
]);

const registerAuthenticationMethod = (authMethod: AuthenticationMethod) => {
if (this.started) {
throw new Error(
Expand Down
36 changes: 36 additions & 0 deletions src/plugins/index_pattern_management/public/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ import { coreMock } from '../../../core/public/mocks';
import { IndexPatternManagementPlugin } from './plugin';
import { urlForwardingPluginMock } from '../../url_forwarding/public/mocks';
import { managementPluginMock } from '../../management/public/mocks';
import {
ManagementApp,
ManagementAppMountParams,
RegisterManagementAppArgs,
} from 'src/plugins/management/public';

describe('DiscoverPlugin', () => {
it('setup successfully', () => {
Expand All @@ -22,4 +27,35 @@ describe('DiscoverPlugin', () => {
expect(setupMock.application.register).toBeCalledTimes(1);
expect(setupMock.chrome.navGroup.addNavLinksToGroup).toBeCalledTimes(5);
});

it('when new navigation is enabled, should navigate to standard IPM app', async () => {
const setupMock = coreMock.createSetup();
const startMock = coreMock.createStart();
setupMock.getStartServices.mockResolvedValue([startMock, {}, {}]);
const initializerContext = coreMock.createPluginInitializerContext();
const pluginInstance = new IndexPatternManagementPlugin(initializerContext);
const managementMock = managementPluginMock.createSetupContract();
let applicationRegistration = {} as Omit<RegisterManagementAppArgs, 'basePath'>;
managementMock.sections.section.opensearchDashboards.registerApp = (
app: Omit<RegisterManagementAppArgs, 'basePath'>
) => {
applicationRegistration = app;
return {} as ManagementApp;
};

setupMock.chrome.navGroup.getNavGroupEnabled.mockReturnValue(true);
startMock.application.getUrlForApp.mockReturnValue('/app/indexPatterns');

pluginInstance.setup(setupMock, {
urlForwarding: urlForwardingPluginMock.createSetupContract(),
management: managementMock,
});

await applicationRegistration.mount({} as ManagementAppMountParams);

expect(startMock.application.getUrlForApp).toBeCalledWith('indexPatterns');
expect(startMock.application.navigateToUrl).toBeCalledWith(
'http://localhost/app/indexPatterns'
);
});
});
Loading
Loading