Skip to content

Commit 824a562

Browse files
ashharrison90ashharrison90
authored
Navigation: share logic between buildBreadcrumbs and usePageTitle (grafana#58819)
* simplify usePageTitle logic a bit * use buildBreadcrumbs logic in usePageTitle * always add home item to navTree, fix some tests * fix remaining unit tests
1 parent 26a7423 commit 824a562

30 files changed

+216
-83
lines changed

Diff for: ‎pkg/services/navtree/navtreeimpl/navtree.go

+6-4
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,7 @@ func (s *ServiceImpl) GetNavTree(c *models.ReqContext, hasEditPerm bool, prefs *
7373
hasAccess := ac.HasAccess(s.accessControl, c)
7474
treeRoot := &navtree.NavTreeRoot{}
7575

76-
if s.features.IsEnabled(featuremgmt.FlagTopnav) {
77-
treeRoot.AddSection(s.getHomeNode(c, prefs))
78-
}
76+
treeRoot.AddSection(s.getHomeNode(c, prefs))
7977

8078
if hasAccess(ac.ReqSignedIn, ac.EvalPermission(dashboards.ActionDashboardsRead)) {
8179
starredItemsLinks, err := s.buildStarredItemsNavLinks(c)
@@ -224,14 +222,18 @@ func (s *ServiceImpl) getHomeNode(c *models.ReqContext, prefs *pref.Preference)
224222
}
225223
}
226224

227-
return &navtree.NavLink{
225+
homeNode := &navtree.NavLink{
228226
Text: "Home",
229227
Id: "home",
230228
Url: homeUrl,
231229
Icon: "home-alt",
232230
Section: navtree.NavSectionCore,
233231
SortWeight: navtree.WeightHome,
234232
}
233+
if !s.features.IsEnabled(featuremgmt.FlagTopnav) {
234+
homeNode.HideFromMenu = true
235+
}
236+
return homeNode
235237
}
236238

237239
func (s *ServiceImpl) addHelpLinks(treeRoot *navtree.NavTreeRoot, c *models.ReqContext) {

Diff for: ‎public/app/core/components/AppChrome/NavToolbar.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ export function NavToolbar({
3333
}: Props) {
3434
const homeNav = useSelector((state) => state.navIndex)[HOME_NAV_ID];
3535
const styles = useStyles2(getStyles);
36-
const breadcrumbs = buildBreadcrumbs(homeNav, sectionNav, pageNav);
36+
const breadcrumbs = buildBreadcrumbs(sectionNav, pageNav, homeNav);
3737

3838
return (
3939
<div className={styles.pageToolbar}>

Diff for: ‎public/app/core/components/Breadcrumbs/utils.test.ts

+7-7
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ describe('breadcrumb utils', () => {
1515
text: 'My section',
1616
url: '/my-section',
1717
};
18-
expect(buildBreadcrumbs(mockHomeNav, sectionNav)).toEqual([{ text: 'My section', href: '/my-section' }]);
18+
expect(buildBreadcrumbs(sectionNav)).toEqual([{ text: 'My section', href: '/my-section' }]);
1919
});
2020

2121
it('includes breadcrumbs for the page nav', () => {
@@ -28,7 +28,7 @@ describe('breadcrumb utils', () => {
2828
text: 'My page',
2929
url: '/my-page',
3030
};
31-
expect(buildBreadcrumbs(mockHomeNav, sectionNav, pageNav)).toEqual([
31+
expect(buildBreadcrumbs(sectionNav, pageNav)).toEqual([
3232
{ text: 'My section', href: '/my-section' },
3333
{ text: 'My page', href: '/my-page' },
3434
]);
@@ -43,7 +43,7 @@ describe('breadcrumb utils', () => {
4343
url: '/my-parent-section',
4444
},
4545
};
46-
expect(buildBreadcrumbs(mockHomeNav, sectionNav)).toEqual([
46+
expect(buildBreadcrumbs(sectionNav)).toEqual([
4747
{ text: 'My parent section', href: '/my-parent-section' },
4848
{ text: 'My section', href: '/my-section' },
4949
]);
@@ -66,7 +66,7 @@ describe('breadcrumb utils', () => {
6666
url: '/my-parent-section',
6767
},
6868
};
69-
expect(buildBreadcrumbs(mockHomeNav, sectionNav, pageNav)).toEqual([
69+
expect(buildBreadcrumbs(sectionNav, pageNav)).toEqual([
7070
{ text: 'My parent section', href: '/my-parent-section' },
7171
{ text: 'My section', href: '/my-section' },
7272
{ text: 'My parent page', href: '/my-parent-page' },
@@ -91,7 +91,7 @@ describe('breadcrumb utils', () => {
9191
url: '/my-parent-section',
9292
},
9393
};
94-
expect(buildBreadcrumbs(mockHomeNav, sectionNav, pageNav)).toEqual([
94+
expect(buildBreadcrumbs(sectionNav, pageNav, mockHomeNav)).toEqual([
9595
{ text: 'Home', href: '/home' },
9696
{ text: 'My page', href: '/my-page' },
9797
]);
@@ -114,7 +114,7 @@ describe('breadcrumb utils', () => {
114114
url: '/my-parent-section',
115115
},
116116
};
117-
expect(buildBreadcrumbs(mockHomeNav, sectionNav, pageNav)).toEqual([
117+
expect(buildBreadcrumbs(sectionNav, pageNav, mockHomeNav)).toEqual([
118118
{ text: 'Home', href: '/home?orgId=1' },
119119
{ text: 'My page', href: '/my-page' },
120120
]);
@@ -137,7 +137,7 @@ describe('breadcrumb utils', () => {
137137
url: '/my-parent-section',
138138
},
139139
};
140-
expect(buildBreadcrumbs(mockHomeNav, sectionNav, pageNav)).toEqual([
140+
expect(buildBreadcrumbs(sectionNav, pageNav, mockHomeNav)).toEqual([
141141
{ text: 'My parent section', href: '/my-parent-section' },
142142
{ text: 'My section', href: '/my-section' },
143143
{ text: 'My parent page', href: '/home?orgId=1&editview=settings' },

Diff for: ‎public/app/core/components/Breadcrumbs/utils.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { getNavTitle } from '../NavBar/navBarItem-translations';
44

55
import { Breadcrumb } from './types';
66

7-
export function buildBreadcrumbs(homeNav: NavModelItem, sectionNav: NavModelItem, pageNav?: NavModelItem) {
7+
export function buildBreadcrumbs(sectionNav: NavModelItem, pageNav?: NavModelItem, homeNav?: NavModelItem) {
88
const crumbs: Breadcrumb[] = [];
99
let foundHome = false;
1010

@@ -17,7 +17,7 @@ export function buildBreadcrumbs(homeNav: NavModelItem, sectionNav: NavModelItem
1717
urlToMatch += `?editview=${urlSearchParams.get('editview')}`;
1818
}
1919
if (!foundHome && !node.hideFromBreadcrumbs) {
20-
if (urlToMatch === homeNav.url) {
20+
if (homeNav && urlToMatch === homeNav.url) {
2121
crumbs.unshift({ text: getNavTitle(homeNav.id) ?? homeNav.text, href: node.url ?? '' });
2222
foundHome = true;
2323
} else {

Diff for: ‎public/app/core/components/NavBar/NavBar.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ export const NavBar = React.memo(() => {
6161
menuOpen
6262
);
6363

64-
const navTree = cloneDeep(navBarTree);
64+
const navTree = cloneDeep(navBarTree).filter((item) => item.hideFromMenu !== true);
6565

6666
const coreItems = navTree
6767
.filter((item) => item.section === NavSection.Core)

Diff for: ‎public/app/core/components/Page/usePageTitle.ts

+11-25
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,23 @@
11
import { useEffect } from 'react';
22

33
import { NavModel, NavModelItem } from '@grafana/data';
4+
import { HOME_NAV_ID } from 'app/core/reducers/navModel';
5+
import { useSelector } from 'app/types';
46

57
import { Branding } from '../Branding/Branding';
8+
import { buildBreadcrumbs } from '../Breadcrumbs/utils';
69

710
export function usePageTitle(navModel?: NavModel, pageNav?: NavModelItem) {
11+
const homeNav = useSelector((state) => state.navIndex)[HOME_NAV_ID];
812
useEffect(() => {
9-
const parts: string[] = [];
10-
if (pageNav) {
11-
if (pageNav.children) {
12-
const activePage = pageNav.children.find((x) => x.active);
13-
if (activePage) {
14-
addTitleSegment(parts, activePage);
15-
}
16-
}
17-
addTitleSegment(parts, pageNav);
18-
}
13+
const sectionNav = (navModel?.node !== navModel?.main ? navModel?.node : navModel?.main) ?? { text: 'Grafana' };
14+
const parts: string[] = buildBreadcrumbs(sectionNav, pageNav, homeNav)
15+
.map((crumb) => crumb.text)
16+
.reverse();
1917

20-
if (navModel) {
21-
if (navModel.node !== navModel.main) {
22-
addTitleSegment(parts, navModel.node);
23-
}
24-
addTitleSegment(parts, navModel.main);
25-
}
26-
27-
parts.push(Branding.AppTitle);
18+
// Override `Home` with the custom brand title
19+
parts[parts.length - 1] = Branding.AppTitle;
2820

2921
document.title = parts.join(' - ');
30-
}, [navModel, pageNav]);
31-
}
32-
33-
function addTitleSegment(parts: string[], node: NavModelItem) {
34-
if (!node.hideFromBreadcrumbs) {
35-
parts.push(node.text);
36-
}
22+
}, [homeNav, navModel, pageNav]);
3723
}

Diff for: ‎public/app/core/components/PageNew/Page.test.tsx

+6-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
66
import { NavModelItem } from '@grafana/data';
77
import { config } from '@grafana/runtime';
88
import { GrafanaContext } from 'app/core/context/GrafanaContext';
9+
import { HOME_NAV_ID } from 'app/core/reducers/navModel';
910
import { configureStore } from 'app/store/configureStore';
1011

1112
import { PageProps } from '../Page/types';
@@ -19,9 +20,12 @@ const pageNav: NavModelItem = {
1920
{ text: 'pageNav child2', url: '2' },
2021
],
2122
};
22-
2323
const setup = (props: Partial<PageProps>) => {
2424
config.bootData.navTree = [
25+
{
26+
id: HOME_NAV_ID,
27+
text: 'Home',
28+
},
2529
{
2630
text: 'Section name',
2731
id: 'section',
@@ -92,7 +96,7 @@ describe('Render', () => {
9296

9397
it('should update document title', async () => {
9498
setup({ navId: 'child1', pageNav });
95-
expect(document.title).toBe('pageNav child1 - pageNav title - Child1 - Section name - Grafana');
99+
expect(document.title).toBe('pageNav title - Child1 - Section name - Grafana');
96100
});
97101

98102
it('should not include hideFromBreadcrumb nodes in title', async () => {

Diff for: ‎public/app/core/navigation/GrafanaRoute.test.tsx

+6-1
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,19 @@
11
import { render, screen } from '@testing-library/react';
22
import React, { ComponentType } from 'react';
3+
import { Provider } from 'react-redux';
34
import { BrowserRouter } from 'react-router-dom';
45
import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
56

67
import { setEchoSrv } from '@grafana/runtime';
78

9+
import { configureStore } from '../../store/configureStore';
810
import { GrafanaContext } from '../context/GrafanaContext';
911
import { Echo } from '../services/echo/Echo';
1012

1113
import { GrafanaRoute, Props } from './GrafanaRoute';
1214

1315
function setup(overrides: Partial<Props>) {
16+
const store = configureStore();
1417
const props: Props = {
1518
location: { search: '?query=hello&test=asd' } as any,
1619
history: {} as any,
@@ -25,7 +28,9 @@ function setup(overrides: Partial<Props>) {
2528
render(
2629
<BrowserRouter>
2730
<GrafanaContext.Provider value={getGrafanaContextMock()}>
28-
<GrafanaRoute {...props} />
31+
<Provider store={store}>
32+
<GrafanaRoute {...props} />
33+
</Provider>
2934
</GrafanaContext.Provider>
3035
</BrowserRouter>
3136
);

Diff for: ‎public/app/core/services/context_srv.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ export class ContextSrv {
7171

7272
constructor() {
7373
if (!config.bootData) {
74-
config.bootData = { user: {}, settings: {} } as any;
74+
config.bootData = { user: {}, settings: {}, navTree: [] } as any;
7575
}
7676

7777
this.user = new User();

Diff for: ‎public/app/features/alerting/AlertRuleList.test.tsx

+9-2
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
import { render, screen } from '@testing-library/react';
22
import userEvent from '@testing-library/user-event';
33
import React from 'react';
4+
import { Provider } from 'react-redux';
45
import { openMenu } from 'react-select-event';
56
import { mockToolkitActionCreator } from 'test/core/redux/mocks';
67

78
import { locationService } from '@grafana/runtime';
89
import { getRouteComponentProps } from 'app/core/navigation/__mocks__/routeProps';
910

1011
import appEvents from '../../core/app_events';
12+
import { configureStore } from '../../store/configureStore';
1113
import { ShowModalReactEvent } from '../../types/events';
1214

1315
import { AlertHowToModal } from './AlertHowToModal';
@@ -29,15 +31,20 @@ const defaultProps: Props = {
2931
};
3032

3133
const setup = (propOverrides?: object) => {
34+
const store = configureStore();
3235
const props: Props = {
3336
...defaultProps,
3437
...propOverrides,
3538
};
3639

37-
const { rerender } = render(<AlertRuleListUnconnected {...props} />);
40+
const { rerender } = render(
41+
<Provider store={store}>
42+
<AlertRuleListUnconnected {...props} />
43+
</Provider>
44+
);
3845

3946
return {
40-
rerender,
47+
rerender: (element: JSX.Element) => rerender(<Provider store={store}>{element}</Provider>),
4148
};
4249
};
4350

Diff for: ‎public/app/features/api-keys/ApiKeysPage.test.tsx

+9-2
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
import { render, screen, within } from '@testing-library/react';
22
import userEvent, { PointerEventsCheckLevel } from '@testing-library/user-event';
33
import React from 'react';
4+
import { Provider } from 'react-redux';
45

56
import { selectors } from '@grafana/e2e-selectors';
67
import { ApiKey, OrgRole } from 'app/types';
78

89
import { mockToolkitActionCreator } from '../../../test/core/redux/mocks';
910
import { silenceConsoleOutput } from '../../../test/core/utils/silenceConsoleOutput';
11+
import { configureStore } from '../../store/configureStore';
1012

1113
import { ApiKeysPageUnconnected, Props } from './ApiKeysPage';
1214
import { getMultipleMockKeys } from './__mocks__/apiKeysMock';
@@ -22,6 +24,7 @@ jest.mock('app/core/core', () => {
2224
});
2325

2426
const setup = (propOverrides: Partial<Props>) => {
27+
const store = configureStore();
2528
const loadApiKeysMock = jest.fn();
2629
const deleteApiKeyMock = jest.fn();
2730
const migrateApiKeyMock = jest.fn();
@@ -54,9 +57,13 @@ const setup = (propOverrides: Partial<Props>) => {
5457

5558
Object.assign(props, propOverrides);
5659

57-
const { rerender } = render(<ApiKeysPageUnconnected {...props} />);
60+
const { rerender } = render(
61+
<Provider store={store}>
62+
<ApiKeysPageUnconnected {...props} />
63+
</Provider>
64+
);
5865
return {
59-
rerender,
66+
rerender: (element: JSX.Element) => rerender(<Provider store={store}>{element}</Provider>),
6067
props,
6168
loadApiKeysMock,
6269
setSearchQueryMock,

Diff for: ‎public/app/features/dashboard/components/DashboardSettings/AnnotationsSettings.test.tsx

+8-3
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { within } from '@testing-library/dom';
22
import { render, screen } from '@testing-library/react';
33
import userEvent from '@testing-library/user-event';
44
import React from 'react';
5+
import { Provider } from 'react-redux';
56
import { BrowserRouter } from 'react-router-dom';
67
import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
78

@@ -10,11 +11,13 @@ import { locationService, setAngularLoader, setDataSourceSrv } from '@grafana/ru
1011
import { GrafanaContext } from 'app/core/context/GrafanaContext';
1112
import { mockDataSource, MockDataSourceSrv } from 'app/features/alerting/unified/mocks';
1213

14+
import { configureStore } from '../../../../store/configureStore';
1315
import { DashboardModel } from '../../state/DashboardModel';
1416

1517
import { AnnotationsSettings } from './AnnotationsSettings';
1618

1719
function setup(dashboard: DashboardModel, editIndex?: number) {
20+
const store = configureStore();
1821
const sectionNav = {
1922
main: { text: 'Dashboard' },
2023
node: {
@@ -24,9 +27,11 @@ function setup(dashboard: DashboardModel, editIndex?: number) {
2427

2528
return render(
2629
<GrafanaContext.Provider value={getGrafanaContextMock()}>
27-
<BrowserRouter>
28-
<AnnotationsSettings sectionNav={sectionNav} dashboard={dashboard} editIndex={editIndex} />
29-
</BrowserRouter>
30+
<Provider store={store}>
31+
<BrowserRouter>
32+
<AnnotationsSettings sectionNav={sectionNav} dashboard={dashboard} editIndex={editIndex} />
33+
</BrowserRouter>
34+
</Provider>
3035
</GrafanaContext.Provider>
3136
);
3237
}

0 commit comments

Comments
 (0)