From 0741f478769dc3a4b1ddbfeb1fb368465ab2af61 Mon Sep 17 00:00:00 2001 From: Ashley Harrison Date: Tue, 18 Apr 2023 14:58:00 +0100 Subject: [PATCH] Navigation: Move `SectionNav` to `AppChrome` (#64391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * move section nav to app chrome * unit tests * Move SectionNav to AppChrome folder * fix duplicate variable rendering --------- Co-authored-by: Torkel Ödegaard --- .betterer.results | 6 +- .../components/AppChrome/AppChrome.test.tsx | 108 ++++++++++++++++++ .../core/components/AppChrome/AppChrome.tsx | 28 ++++- .../AppChrome/AppChromeService.test.tsx | 19 ++- .../components/AppChrome/AppChromeService.tsx | 13 ++- .../components/AppChrome/AppChromeUpdate.tsx | 6 +- .../SectionNav}/SectionNav.tsx | 0 .../SectionNav}/SectionNavItem.test.tsx | 0 .../SectionNav}/SectionNavItem.tsx | 0 .../SectionNav}/SectionNavToggle.tsx | 0 public/app/core/components/Page/Page.tsx | 2 +- .../app/core/components/PageNew/Page.test.tsx | 27 +---- public/app/core/components/PageNew/Page.tsx | 61 ++++------ .../DashboardSettings.test.tsx | 38 ------ public/app/features/explore/ExplorePage.tsx | 2 +- 15 files changed, 190 insertions(+), 120 deletions(-) create mode 100644 public/app/core/components/AppChrome/AppChrome.test.tsx rename public/app/core/components/{PageNew => AppChrome/SectionNav}/SectionNav.tsx (100%) rename public/app/core/components/{PageNew => AppChrome/SectionNav}/SectionNavItem.test.tsx (100%) rename public/app/core/components/{PageNew => AppChrome/SectionNav}/SectionNavItem.tsx (100%) rename public/app/core/components/{PageNew => AppChrome/SectionNav}/SectionNavToggle.tsx (100%) delete mode 100644 public/app/features/dashboard/components/DashboardSettings/DashboardSettings.test.tsx diff --git a/.betterer.results b/.betterer.results index 5608c47eebe..23a5eea677e 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1548,6 +1548,9 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "5"], [0, 0, 0, "Unexpected any. Specify a different type.", "6"] ], + "public/app/core/components/AppChrome/SectionNav/SectionNavItem.tsx:5381": [ + [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"] + ], "public/app/core/components/DynamicImports/SafeDynamicImport.tsx:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"] ], @@ -1650,9 +1653,6 @@ exports[`better eslint`] = { [0, 0, 0, "Do not use any type assertions.", "85"], [0, 0, 0, "Unexpected any. Specify a different type.", "86"] ], - "public/app/core/components/PageNew/SectionNavItem.tsx:5381": [ - [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"] - ], "public/app/core/components/PanelTypeFilter/PanelTypeFilter.tsx:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], [0, 0, 0, "Unexpected any. Specify a different type.", "1"] diff --git a/public/app/core/components/AppChrome/AppChrome.test.tsx b/public/app/core/components/AppChrome/AppChrome.test.tsx new file mode 100644 index 00000000000..6ffcb0c0b88 --- /dev/null +++ b/public/app/core/components/AppChrome/AppChrome.test.tsx @@ -0,0 +1,108 @@ +import { render, screen } from '@testing-library/react'; +import { KBarProvider } from 'kbar'; +import React, { ReactNode } from 'react'; +import { TestProvider } from 'test/helpers/TestProvider'; +import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock'; + +import { ArrayVector, DataFrame, DataFrameView, FieldType, NavModelItem } from '@grafana/data'; +import { config } from '@grafana/runtime'; +import { HOME_NAV_ID } from 'app/core/reducers/navModel'; +import { DashboardQueryResult, getGrafanaSearcher, QueryResponse } from 'app/features/search/service'; + +import { Page } from '../PageNew/Page'; + +import { AppChrome } from './AppChrome'; + +const pageNav: NavModelItem = { + text: 'pageNav title', + children: [ + { text: 'pageNav child1', url: '1', active: true }, + { text: 'pageNav child2', url: '2' }, + ], +}; + +const searchData: DataFrame = { + fields: [ + { name: 'kind', type: FieldType.string, config: {}, values: new ArrayVector([]) }, + { name: 'name', type: FieldType.string, config: {}, values: new ArrayVector([]) }, + { name: 'uid', type: FieldType.string, config: {}, values: new ArrayVector([]) }, + { name: 'url', type: FieldType.string, config: {}, values: new ArrayVector([]) }, + { name: 'tags', type: FieldType.other, config: {}, values: new ArrayVector([]) }, + { name: 'location', type: FieldType.string, config: {}, values: new ArrayVector([]) }, + ], + length: 0, +}; + +const mockSearchResult: QueryResponse = { + isItemLoaded: jest.fn(), + loadMoreItems: jest.fn(), + totalRows: searchData.length, + view: new DataFrameView(searchData), +}; + +const setup = (children: ReactNode) => { + config.bootData.navTree = [ + { + id: HOME_NAV_ID, + text: 'Home', + }, + { + text: 'Section name', + id: 'section', + url: 'section', + children: [ + { text: 'Child1', id: 'child1', url: 'section/child1' }, + { text: 'Child2', id: 'child2', url: 'section/child2' }, + ], + }, + ]; + + const context = getGrafanaContextMock(); + + const renderResult = render( + + + +
{children}
+ + + + ); + + return { renderResult, context }; +}; + +describe('AppChrome', () => { + beforeAll(() => { + // need to mock out the search service since kbar calls it to fetch recent dashboards + jest.spyOn(getGrafanaSearcher(), 'search').mockResolvedValue(mockSearchResult); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should render section nav model based on navId', async () => { + setup(Children); + expect(await screen.findByTestId('page-children')).toBeInTheDocument(); + + expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument(); + expect(screen.getAllByRole('tab').length).toBe(3); + }); + + it('should render section nav model based on navId and item page nav', async () => { + setup( + + Children + + ); + expect(await screen.findByTestId('page-children')).toBeInTheDocument(); + + expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument(); + expect(screen.getByRole('heading', { name: 'pageNav title' })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'Tab pageNav child1' })).toBeInTheDocument(); + }); +}); diff --git a/public/app/core/components/AppChrome/AppChrome.tsx b/public/app/core/components/AppChrome/AppChrome.tsx index 829ca792805..2ab7cc8ad84 100644 --- a/public/app/core/components/AppChrome/AppChrome.tsx +++ b/public/app/core/components/AppChrome/AppChrome.tsx @@ -1,7 +1,7 @@ import { css, cx } from '@emotion/css'; import React, { PropsWithChildren } from 'react'; -import { GrafanaTheme2 } from '@grafana/data'; +import { GrafanaTheme2, PageLayoutType } from '@grafana/data'; import { useStyles2 } from '@grafana/ui'; import { useGrafana } from 'app/core/context/GrafanaContext'; import { CommandPalette } from 'app/features/commandPalette/CommandPalette'; @@ -9,6 +9,7 @@ import { KioskMode } from 'app/types'; import { MegaMenu } from './MegaMenu/MegaMenu'; import { NavToolbar } from './NavToolbar/NavToolbar'; +import { SectionNav } from './SectionNav/SectionNav'; import { TopSearchBar } from './TopBar/TopSearchBar'; import { TOP_BAR_LEVEL_HEIGHT } from './types'; @@ -38,7 +39,7 @@ export function AppChrome({ children }: Props) { {!searchBarHidden && } )} -
{children}
+
+
+ {state.layout === PageLayoutType.Standard && state.sectionNav && } +
{children}
+
+
{!state.chromeless && ( <> chrome.setMegaMenu(false)} /> @@ -88,5 +94,21 @@ const getStyles = (theme: GrafanaTheme2) => { flexDirection: 'column', borderBottom: `1px solid ${theme.colors.border.weak}`, }), + panes: css({ + label: 'page-panes', + display: 'flex', + height: '100%', + width: '100%', + flexGrow: 1, + minHeight: 0, + flexDirection: 'column', + [theme.breakpoints.up('md')]: { + flexDirection: 'row', + }, + }), + pageContainer: css({ + label: 'page-container', + flexGrow: 1, + }), }; }; diff --git a/public/app/core/components/AppChrome/AppChromeService.test.tsx b/public/app/core/components/AppChrome/AppChromeService.test.tsx index 050d1b25eab..d6021981a88 100644 --- a/public/app/core/components/AppChrome/AppChromeService.test.tsx +++ b/public/app/core/components/AppChrome/AppChromeService.test.tsx @@ -13,25 +13,34 @@ describe('AppChromeService', () => { let stateChanges = 0; chromeService.state.subscribe(() => stateChanges++); - chromeService.update({ sectionNav: { text: 'hello' }, pageNav: { text: 'test', url: 'A' } }); - chromeService.update({ sectionNav: { text: 'hello' }, pageNav: { text: 'test', url: 'A' } }); + chromeService.update({ + sectionNav: { node: { text: 'hello' }, main: { text: '' } }, + pageNav: { text: 'test', url: 'A' }, + }); + chromeService.update({ + sectionNav: { node: { text: 'hello' }, main: { text: '' } }, + pageNav: { text: 'test', url: 'A' }, + }); expect(stateChanges).toBe(2); // if url change we should update - chromeService.update({ sectionNav: { text: 'hello' }, pageNav: { text: 'test', url: 'new/url' } }); + chromeService.update({ + sectionNav: { node: { text: 'hello' }, main: { text: '' } }, + pageNav: { text: 'test', url: 'new/url' }, + }); expect(stateChanges).toBe(3); // if active child changed should update state chromeService.update({ - sectionNav: { text: 'hello' }, + sectionNav: { node: { text: 'hello' }, main: { text: '' } }, pageNav: { text: 'test', url: 'A', children: [{ text: 'child', active: true }] }, }); expect(stateChanges).toBe(4); // If active child is the same we should not update state chromeService.update({ - sectionNav: { text: 'hello' }, + sectionNav: { node: { text: 'hello' }, main: { text: '' } }, pageNav: { text: 'test', url: 'A', children: [{ text: 'child', active: true }] }, }); expect(stateChanges).toBe(4); diff --git a/public/app/core/components/AppChrome/AppChromeService.tsx b/public/app/core/components/AppChrome/AppChromeService.tsx index 641d4d87283..f2fa78f168f 100644 --- a/public/app/core/components/AppChrome/AppChromeService.tsx +++ b/public/app/core/components/AppChrome/AppChromeService.tsx @@ -1,7 +1,7 @@ import { useObservable } from 'react-use'; import { BehaviorSubject } from 'rxjs'; -import { AppEvents, NavModelItem, UrlQueryValue } from '@grafana/data'; +import { AppEvents, NavModel, NavModelItem, PageLayoutType, UrlQueryValue } from '@grafana/data'; import { locationService, reportInteraction } from '@grafana/runtime'; import appEvents from 'app/core/app_events'; import { t } from 'app/core/internationalization'; @@ -13,12 +13,13 @@ import { RouteDescriptor } from '../../navigation/types'; export interface AppChromeState { chromeless?: boolean; - sectionNav: NavModelItem; + sectionNav: NavModel; pageNav?: NavModelItem; actions?: React.ReactNode; searchBarHidden?: boolean; megaMenuOpen?: boolean; kioskMode: KioskMode | null; + layout: PageLayoutType; } export class AppChromeService { @@ -28,9 +29,10 @@ export class AppChromeService { readonly state = new BehaviorSubject({ chromeless: true, // start out hidden to not flash it on pages without chrome - sectionNav: { text: t('nav.home.title', 'Home') }, + sectionNav: { node: { text: t('nav.home.title', 'Home') }, main: { text: '' } }, searchBarHidden: store.getBool(this.searchBarStorageKey, false), kioskMode: null, + layout: PageLayoutType.Standard, }); setMatchedRoute(route: RouteDescriptor) { @@ -50,8 +52,9 @@ export class AppChromeService { if (!this.routeChangeHandled) { newState.actions = undefined; newState.pageNav = undefined; - newState.sectionNav = { text: t('nav.home.title', 'Home') }; + newState.sectionNav = { node: { text: t('nav.home.title', 'Home') }, main: { text: '' } }; newState.chromeless = this.currentRoute?.chromeless; + newState.layout = PageLayoutType.Standard; this.routeChangeHandled = true; } @@ -74,7 +77,7 @@ export class AppChromeService { if (newState.sectionNav !== current.sectionNav || newState.pageNav !== current.pageNav) { if ( newState.actions === current.actions && - navItemsAreTheSame(newState.sectionNav, current.sectionNav) && + navItemsAreTheSame(newState.sectionNav.node, current.sectionNav.node) && navItemsAreTheSame(newState.pageNav, current.pageNav) ) { return true; diff --git a/public/app/core/components/AppChrome/AppChromeUpdate.tsx b/public/app/core/components/AppChrome/AppChromeUpdate.tsx index 00644c83b67..7a654d5fc74 100644 --- a/public/app/core/components/AppChrome/AppChromeUpdate.tsx +++ b/public/app/core/components/AppChrome/AppChromeUpdate.tsx @@ -1,4 +1,4 @@ -import React, { useEffect } from 'react'; +import React, { useLayoutEffect } from 'react'; import { useGrafana } from 'app/core/context/GrafanaContext'; @@ -12,7 +12,9 @@ export interface AppChromeUpdateProps { export const AppChromeUpdate = React.memo(({ actions }: AppChromeUpdateProps) => { const { chrome } = useGrafana(); - useEffect(() => { + // We use useLayoutEffect here to make sure that the chrome is updated before the page is rendered + // This prevents flickering actions when going from one dashbaord to another for example + useLayoutEffect(() => { chrome.update({ actions }); }); return null; diff --git a/public/app/core/components/PageNew/SectionNav.tsx b/public/app/core/components/AppChrome/SectionNav/SectionNav.tsx similarity index 100% rename from public/app/core/components/PageNew/SectionNav.tsx rename to public/app/core/components/AppChrome/SectionNav/SectionNav.tsx diff --git a/public/app/core/components/PageNew/SectionNavItem.test.tsx b/public/app/core/components/AppChrome/SectionNav/SectionNavItem.test.tsx similarity index 100% rename from public/app/core/components/PageNew/SectionNavItem.test.tsx rename to public/app/core/components/AppChrome/SectionNav/SectionNavItem.test.tsx diff --git a/public/app/core/components/PageNew/SectionNavItem.tsx b/public/app/core/components/AppChrome/SectionNav/SectionNavItem.tsx similarity index 100% rename from public/app/core/components/PageNew/SectionNavItem.tsx rename to public/app/core/components/AppChrome/SectionNav/SectionNavItem.tsx diff --git a/public/app/core/components/PageNew/SectionNavToggle.tsx b/public/app/core/components/AppChrome/SectionNav/SectionNavToggle.tsx similarity index 100% rename from public/app/core/components/PageNew/SectionNavToggle.tsx rename to public/app/core/components/AppChrome/SectionNav/SectionNavToggle.tsx diff --git a/public/app/core/components/Page/Page.tsx b/public/app/core/components/Page/Page.tsx index 4931ade57c4..9d5b594809b 100644 --- a/public/app/core/components/Page/Page.tsx +++ b/public/app/core/components/Page/Page.tsx @@ -44,7 +44,7 @@ export const OldPage: PageType = ({ if (navModel) { // This is needed for chrome to update it's chromeless state chrome.update({ - sectionNav: navModel.node, + sectionNav: navModel, }); } else { // Need to trigger a chrome state update for the route change to be processed diff --git a/public/app/core/components/PageNew/Page.test.tsx b/public/app/core/components/PageNew/Page.test.tsx index 2492edf45e6..53068a201de 100644 --- a/public/app/core/components/PageNew/Page.test.tsx +++ b/public/app/core/components/PageNew/Page.test.tsx @@ -3,7 +3,7 @@ import React from 'react'; import { TestProvider } from 'test/helpers/TestProvider'; import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock'; -import { NavModelItem } from '@grafana/data'; +import { NavModelItem, PageLayoutType } from '@grafana/data'; import { config } from '@grafana/runtime'; import { HOME_NAV_ID } from 'app/core/reducers/navModel'; @@ -65,28 +65,11 @@ describe('Render', () => { expect(screen.getAllByRole('tab').length).toBe(2); }); - it('should render section nav model based on navId', async () => { - setup({ navId: 'child1' }); - - expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument(); - expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument(); - expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument(); - expect(screen.getAllByRole('tab').length).toBe(3); - }); - - it('should update chrome with section and pageNav', async () => { - const { context } = setup({ navId: 'child1', pageNav }); - expect(context.chrome.state.getValue().sectionNav.id).toBe('child1'); + it('should update chrome with section, pageNav and layout', async () => { + const { context } = setup({ navId: 'child1', pageNav, layout: PageLayoutType.Canvas }); + expect(context.chrome.state.getValue().sectionNav.node.id).toBe('child1'); expect(context.chrome.state.getValue().pageNav).toBe(pageNav); - }); - - it('should render section nav model based on navId and item page nav', async () => { - setup({ navId: 'child1', pageNav }); - - expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument(); - expect(screen.getByRole('heading', { name: 'pageNav title' })).toBeInTheDocument(); - expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument(); - expect(screen.getByRole('tab', { name: 'Tab pageNav child1' })).toBeInTheDocument(); + expect(context.chrome.state.getValue().layout).toBe(PageLayoutType.Canvas); }); it('should update document title', async () => { diff --git a/public/app/core/components/PageNew/Page.tsx b/public/app/core/components/PageNew/Page.tsx index 85993de8d13..fd8e3c5f65e 100644 --- a/public/app/core/components/PageNew/Page.tsx +++ b/public/app/core/components/PageNew/Page.tsx @@ -1,6 +1,6 @@ // Libraries import { css, cx } from '@emotion/css'; -import React, { useEffect } from 'react'; +import React, { useLayoutEffect } from 'react'; import { GrafanaTheme2, PageLayoutType } from '@grafana/data'; import { CustomScrollbar, useStyles2 } from '@grafana/ui'; @@ -13,7 +13,6 @@ import { usePageTitle } from '../Page/usePageTitle'; import { PageContents } from './PageContents'; import { PageHeader } from './PageHeader'; import { PageTabs } from './PageTabs'; -import { SectionNav } from './SectionNav'; export const Page: PageType = ({ navId, @@ -39,38 +38,36 @@ export const Page: PageType = ({ const pageHeaderNav = pageNav ?? navModel?.node; - useEffect(() => { + // We use useLayoutEffect here to make sure that the chrome is updated before the page is rendered + // This prevents flickering sectionNav when going from dashbaord to settings for example + useLayoutEffect(() => { if (navModel) { chrome.update({ - sectionNav: navModel.node, + sectionNav: navModel, pageNav: pageNav, + layout: layout, }); } - }, [navModel, pageNav, chrome]); + }, [navModel, pageNav, chrome, layout]); return (
{layout === PageLayoutType.Standard && ( -
- {navModel && } -
- -
- {pageHeaderNav && ( - - )} - {pageNav && pageNav.children && } -
{children}
-
- + +
+ {pageHeaderNav && ( + + )} + {pageNav && pageNav.children && } +
{children}
-
+ )} {layout === PageLayoutType.Canvas && ( @@ -102,22 +99,6 @@ const getStyles = (theme: GrafanaTheme2) => { flexDirection: 'column', minHeight: 0, }), - panes: css({ - label: 'page-panes', - display: 'flex', - height: '100%', - width: '100%', - flexGrow: 1, - minHeight: 0, - flexDirection: 'column', - [theme.breakpoints.up('md')]: { - flexDirection: 'row', - }, - }), - pageContainer: css({ - label: 'page-container', - flexGrow: 1, - }), pageContent: css({ label: 'page-content', flexGrow: 1, diff --git a/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.test.tsx b/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.test.tsx deleted file mode 100644 index 4dcb106fb0e..00000000000 --- a/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.test.tsx +++ /dev/null @@ -1,38 +0,0 @@ -import { render, screen } from '@testing-library/react'; -import React from 'react'; -import { TestProvider } from 'test/helpers/TestProvider'; - -import { NavModel, NavModelItem } from '@grafana/data'; -import { BackendSrv, setBackendSrv } from '@grafana/runtime'; - -import { createDashboardModelFixture } from '../../state/__fixtures__/dashboardFixtures'; - -import { DashboardSettings } from './DashboardSettings'; - -setBackendSrv({ - get: jest.fn().mockResolvedValue([]), -} as unknown as BackendSrv); - -describe('DashboardSettings', () => { - it('pressing escape navigates away correctly', async () => { - const dashboard = createDashboardModelFixture( - { - title: 'Foo', - }, - { - folderId: 1, - } - ); - - const sectionNav: NavModel = { main: { text: 'Dashboards' }, node: { text: 'Dashboards' } }; - const pageNav: NavModelItem = { text: 'My cool dashboard' }; - - render( - - - - ); - - expect(await screen.findByRole('tab', { name: 'Tab Settings' })).toBeInTheDocument(); - }); -}); diff --git a/public/app/features/explore/ExplorePage.tsx b/public/app/features/explore/ExplorePage.tsx index 10400900e90..68458025230 100644 --- a/public/app/features/explore/ExplorePage.tsx +++ b/public/app/features/explore/ExplorePage.tsx @@ -49,7 +49,7 @@ export function ExplorePage(props: GrafanaRouteComponentProps<{}, ExploreQueryPa useEffect(() => { //This is needed for breadcrumbs and topnav. //We should probably abstract this out at some point - chrome.update({ sectionNav: navModel.node }); + chrome.update({ sectionNav: navModel }); }, [chrome, navModel]); useEffect(() => {