Navigation: Unify Page component (#66951)

* remove old page component

* add test to check initDashboard is only called once (prevent variables loading twice)

* add help node

* update unit tests

* remove last mentions of topnav

* fix unit tests

* remove unused props from ButtonRow interface

* remove prop from test

public/app/core/components/AppChrome/AppChrome.test.tsx

@@ -9,7 +9,7 @@ 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 { Page } from '../Page/Page';
 
 import { AppChrome } from './AppChrome';
 

public/app/core/components/Page/Page.test.tsx

@@ -1,23 +1,28 @@
 import { render, screen } from '@testing-library/react';
 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';
 
 import { Page } from './Page';
 import { PageProps } from './types';
 
 const pageNav: NavModelItem = {
-  text: 'Main title',
+  text: 'pageNav title',
   children: [
-    { text: 'Child1', url: '1', active: true },
-    { text: 'Child2', url: '2' },
+    { text: 'pageNav child1', url: '1', active: true },
+    { text: 'pageNav child2', url: '2' },
   ],
 };
-
 const setup = (props: Partial<PageProps>) => {
   config.bootData.navTree = [
+    {
+      id: HOME_NAV_ID,
+      text: 'Home',
+    },
     {
       text: 'Section name',
       id: 'section',
@@ -29,13 +34,17 @@ const setup = (props: Partial<PageProps>) => {
     },
   ];
 
-  return render(
-    <TestProvider>
+  const context = getGrafanaContextMock();
+
+  const renderResult = render(
+    <TestProvider grafanaContext={context}>
       <Page {...props}>
         <div data-testid="page-children">Children</div>
       </Page>
     </TestProvider>
   );
+
+  return { renderResult, context };
 };
 
 describe('Render', () => {
@@ -51,14 +60,25 @@ describe('Render', () => {
   it('should render header when pageNav supplied', async () => {
     setup({ pageNav });
 
-    expect(screen.getByRole('heading', { name: 'Main title' })).toBeInTheDocument();
+    expect(screen.getByRole('heading', { name: 'pageNav title' })).toBeInTheDocument();
     expect(screen.getAllByRole('tab').length).toBe(2);
   });
 
-  it('should get header nav model from redux navIndex', async () => {
-    setup({ navId: '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);
+    expect(context.chrome.state.getValue().layout).toBe(PageLayoutType.Canvas);
+  });
 
-    expect(screen.getByRole('heading', { name: 'Section name' })).toBeInTheDocument();
-    expect(screen.getAllByRole('tab').length).toBe(2);
+  it('should update document title', async () => {
+    setup({ navId: 'child1', pageNav });
+    expect(document.title).toBe('pageNav title - Child1 - Section name - Grafana');
+  });
+
+  it('should not include hideFromBreadcrumb nodes in title', async () => {
+    pageNav.children![0].hideFromBreadcrumbs = true;
+    setup({ navId: 'child1', pageNav });
+    expect(document.title).toBe('pageNav title - Child1 - Section name - Grafana');
   });
 });

public/app/core/components/Page/Page.tsx

@@ -1,35 +1,32 @@
 // Libraries
 import { css, cx } from '@emotion/css';
-import React, { useEffect } from 'react';
+import React, { useLayoutEffect } from 'react';
 
 import { GrafanaTheme2, PageLayoutType } from '@grafana/data';
-import { config } from '@grafana/runtime';
 import { CustomScrollbar, useStyles2 } from '@grafana/ui';
 import { useGrafana } from 'app/core/context/GrafanaContext';
 
-import { Footer } from '../Footer/Footer';
-import { PageHeader } from '../PageHeader/PageHeader';
-import { Page as NewPage } from '../PageNew/Page';
-
 import { PageContents } from './PageContents';
+import { PageHeader } from './PageHeader';
+import { PageTabs } from './PageTabs';
 import { PageType } from './types';
 import { usePageNav } from './usePageNav';
 import { usePageTitle } from './usePageTitle';
 
-export const OldPage: PageType = ({
+export const Page: PageType = ({
   navId,
   navModel: oldNavProp,
   pageNav,
+  renderTitle,
+  actions,
+  subTitle,
   children,
   className,
-  toolbar,
-  scrollRef,
-  scrollTop,
-  layout = PageLayoutType.Standard,
-  renderTitle,
-  subTitle,
-  actions,
   info,
+  layout = PageLayoutType.Standard,
+  toolbar,
+  scrollTop,
+  scrollRef,
   ...otherProps
 }) => {
   const styles = useStyles2(getStyles);
@@ -38,48 +35,46 @@ export const OldPage: PageType = ({
 
   usePageTitle(navModel, pageNav);
 
-  const pageHeaderNav = pageNav ?? navModel?.main;
+  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) {
-      // This is needed for chrome to update it's chromeless state
       chrome.update({
         sectionNav: navModel,
+        pageNav: pageNav,
+        layout: layout,
       });
-    } else {
-      // Need to trigger a chrome state update for the route change to be processed
-      chrome.update({});
     }
-  }, [navModel, chrome]);
+  }, [navModel, pageNav, chrome, layout]);
 
   return (
     <div className={cx(styles.wrapper, className)} {...otherProps}>
       {layout === PageLayoutType.Standard && (
         <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
-          <div className={cx('page-scrollbar-content', className)}>
+          <div className={styles.pageInner}>
             {pageHeaderNav && (
               <PageHeader
                 actions={actions}
-                info={info}
                 navItem={pageHeaderNav}
                 renderTitle={renderTitle}
+                info={info}
                 subTitle={subTitle}
               />
             )}
-            {children}
-            <Footer />
+            {pageNav && pageNav.children && <PageTabs navItem={pageNav} />}
+            <div className={styles.pageContent}>{children}</div>
           </div>
         </CustomScrollbar>
       )}
       {layout === PageLayoutType.Canvas && (
-        <>
-          {toolbar}
-          <div className={styles.scrollWrapper}>
-            <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
-              <div className={cx(styles.content, !toolbar && styles.contentWithoutToolbar)}>{children}</div>
-            </CustomScrollbar>
+        <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
+          <div className={styles.canvasContent}>
+            {toolbar}
+            {children}
           </div>
-        </>
+        </CustomScrollbar>
       )}
       {layout === PageLayoutType.Custom && (
         <>
@@ -91,33 +86,46 @@ export const OldPage: PageType = ({
   );
 };
 
-OldPage.Contents = PageContents;
+Page.Contents = PageContents;
 
-export const Page: PageType = config.featureToggles.topnav ? NewPage : OldPage;
+const getStyles = (theme: GrafanaTheme2) => {
+  return {
+    wrapper: css({
+      label: 'page-wrapper',
+      height: '100%',
+      display: 'flex',
+      flex: '1 1 0',
+      flexDirection: 'column',
+      minHeight: 0,
+    }),
+    pageContent: css({
+      label: 'page-content',
+      flexGrow: 1,
+    }),
+    pageInner: css({
+      label: 'page-inner',
+      padding: theme.spacing(2),
+      borderRadius: theme.shape.borderRadius(1),
+      border: `1px solid ${theme.colors.border.weak}`,
+      borderBottom: 'none',
+      background: theme.colors.background.primary,
+      display: 'flex',
+      flexDirection: 'column',
+      flexGrow: 1,
+      margin: theme.spacing(0, 0, 0, 0),
 
-const getStyles = (theme: GrafanaTheme2) => ({
-  wrapper: css({
-    width: '100%',
-    height: '100%',
-    display: 'flex',
-    flex: '1 1 0',
-    flexDirection: 'column',
-    minHeight: 0,
-  }),
-  scrollWrapper: css({
-    width: '100%',
-    flexGrow: 1,
-    minHeight: 0,
-    display: 'flex',
-  }),
-  content: css({
-    display: 'flex',
-    flexDirection: 'column',
-    padding: theme.spacing(0, 2, 2, 2),
-    flexBasis: '100%',
-    flexGrow: 1,
-  }),
-  contentWithoutToolbar: css({
-    padding: theme.spacing(2),
-  }),
-});
+      [theme.breakpoints.up('md')]: {
+        margin: theme.spacing(2, 2, 0, 1),
+        padding: theme.spacing(3),
+      },
+    }),
+    canvasContent: css({
+      label: 'canvas-content',
+      display: 'flex',
+      flexDirection: 'column',
+      padding: theme.spacing(2),
+      flexBasis: '100%',
+      flexGrow: 1,
+    }),
+  };
+};

public/app/core/components/Page/PageContents.tsx

@@ -1,8 +1,6 @@
 // Libraries
-import { cx } from '@emotion/css';
 import React from 'react';
 
-// Components
 import PageLoader from '../PageLoader/PageLoader';
 
 interface Props {
@@ -12,5 +10,7 @@ interface Props {
 }
 
 export const PageContents = ({ isLoading, children, className }: Props) => {
-  return <div className={cx('page-container', 'page-body', className)}>{isLoading ? <PageLoader /> : children}</div>;
+  let content = className ? <div className={className}>{children}</div> : children;
+
+  return <>{isLoading ? <PageLoader /> : content}</>;
 };

public/app/core/components/Page/PageHeader.tsx

@@ -4,9 +4,10 @@ import React from 'react';
 import { NavModelItem, GrafanaTheme2 } from '@grafana/data';
 import { useStyles2 } from '@grafana/ui';
 
-import { PageInfoItem } from '../Page/types';
 import { PageInfo } from '../PageInfo/PageInfo';
 
+import { PageInfoItem } from './types';
+
 export interface Props {
   navItem: NavModelItem;
   renderTitle?: (title: string) => React.ReactNode;

public/app/core/components/Page/PluginPage.tsx

@@ -3,7 +3,7 @@ import React, { useContext } from 'react';
 import { PluginPageProps } from '@grafana/runtime';
 import { PluginPageContext } from 'app/features/plugins/components/PluginPageContext';
 
-import { Page } from '../Page/Page';
+import { Page } from './Page';
 
 export function PluginPage({ actions, children, info, pageNav, layout, renderTitle, subTitle }: PluginPageProps) {
   const context = useContext(PluginPageContext);

public/app/core/components/PageNew/Page.test.tsx

@@ -1,85 +0,0 @@
-import { render, screen } from '@testing-library/react';
-import React from 'react';
-import { TestProvider } from 'test/helpers/TestProvider';
-import { getGrafanaContextMock } from 'test/mocks/getGrafanaContextMock';
-
-import { NavModelItem, PageLayoutType } from '@grafana/data';
-import { config } from '@grafana/runtime';
-import { HOME_NAV_ID } from 'app/core/reducers/navModel';
-
-import { PageProps } from '../Page/types';
-
-import { Page } from './Page';
-
-const pageNav: NavModelItem = {
-  text: 'pageNav title',
-  children: [
-    { text: 'pageNav child1', url: '1', active: true },
-    { text: 'pageNav child2', url: '2' },
-  ],
-};
-const setup = (props: Partial<PageProps>) => {
-  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(
-    <TestProvider grafanaContext={context}>
-      <Page {...props}>
-        <div data-testid="page-children">Children</div>
-      </Page>
-    </TestProvider>
-  );
-
-  return { renderResult, context };
-};
-
-describe('Render', () => {
-  it('should render component with emtpy Page container', async () => {
-    setup({});
-    const children = await screen.findByTestId('page-children');
-    expect(children).toBeInTheDocument();
-
-    const pageHeader = screen.queryByRole('heading');
-    expect(pageHeader).not.toBeInTheDocument();
-  });
-
-  it('should render header when pageNav supplied', async () => {
-    setup({ pageNav });
-
-    expect(screen.getByRole('heading', { name: 'pageNav title' })).toBeInTheDocument();
-    expect(screen.getAllByRole('tab').length).toBe(2);
-  });
-
-  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);
-    expect(context.chrome.state.getValue().layout).toBe(PageLayoutType.Canvas);
-  });
-
-  it('should update document title', async () => {
-    setup({ navId: 'child1', pageNav });
-    expect(document.title).toBe('pageNav title - Child1 - Section name - Grafana');
-  });
-
-  it('should not include hideFromBreadcrumb nodes in title', async () => {
-    pageNav.children![0].hideFromBreadcrumbs = true;
-    setup({ navId: 'child1', pageNav });
-    expect(document.title).toBe('pageNav title - Child1 - Section name - Grafana');
-  });
-});

public/app/core/components/PageNew/Page.tsx

@@ -1,132 +0,0 @@
-// Libraries
-import { css, cx } from '@emotion/css';
-import React, { useLayoutEffect } from 'react';
-
-import { GrafanaTheme2, PageLayoutType } from '@grafana/data';
-import { CustomScrollbar, useStyles2 } from '@grafana/ui';
-import { useGrafana } from 'app/core/context/GrafanaContext';
-
-import { PageType } from '../Page/types';
-import { usePageNav } from '../Page/usePageNav';
-import { usePageTitle } from '../Page/usePageTitle';
-
-import { PageContents } from './PageContents';
-import { PageHeader } from './PageHeader';
-import { PageTabs } from './PageTabs';
-
-export const Page: PageType = ({
-  navId,
-  navModel: oldNavProp,
-  pageNav,
-  renderTitle,
-  actions,
-  subTitle,
-  children,
-  className,
-  info,
-  layout = PageLayoutType.Standard,
-  toolbar,
-  scrollTop,
-  scrollRef,
-  ...otherProps
-}) => {
-  const styles = useStyles2(getStyles);
-  const navModel = usePageNav(navId, oldNavProp);
-  const { chrome } = useGrafana();
-
-  usePageTitle(navModel, pageNav);
-
-  const pageHeaderNav = pageNav ?? navModel?.node;
-
-  // 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,
-        pageNav: pageNav,
-        layout: layout,
-      });
-    }
-  }, [navModel, pageNav, chrome, layout]);
-
-  return (
-    <div className={cx(styles.wrapper, className)} {...otherProps}>
-      {layout === PageLayoutType.Standard && (
-        <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
-          <div className={styles.pageInner}>
-            {pageHeaderNav && (
-              <PageHeader
-                actions={actions}
-                navItem={pageHeaderNav}
-                renderTitle={renderTitle}
-                info={info}
-                subTitle={subTitle}
-              />
-            )}
-            {pageNav && pageNav.children && <PageTabs navItem={pageNav} />}
-            <div className={styles.pageContent}>{children}</div>
-          </div>
-        </CustomScrollbar>
-      )}
-      {layout === PageLayoutType.Canvas && (
-        <CustomScrollbar autoHeightMin={'100%'} scrollTop={scrollTop} scrollRefCallback={scrollRef}>
-          <div className={styles.canvasContent}>
-            {toolbar}
-            {children}
-          </div>
-        </CustomScrollbar>
-      )}
-      {layout === PageLayoutType.Custom && (
-        <>
-          {toolbar}
-          {children}
-        </>
-      )}
-    </div>
-  );
-};
-
-Page.Contents = PageContents;
-
-const getStyles = (theme: GrafanaTheme2) => {
-  return {
-    wrapper: css({
-      label: 'page-wrapper',
-      height: '100%',
-      display: 'flex',
-      flex: '1 1 0',
-      flexDirection: 'column',
-      minHeight: 0,
-    }),
-    pageContent: css({
-      label: 'page-content',
-      flexGrow: 1,
-    }),
-    pageInner: css({
-      label: 'page-inner',
-      padding: theme.spacing(2),
-      borderRadius: theme.shape.borderRadius(1),
-      border: `1px solid ${theme.colors.border.weak}`,
-      borderBottom: 'none',
-      background: theme.colors.background.primary,
-      display: 'flex',
-      flexDirection: 'column',
-      flexGrow: 1,
-      margin: theme.spacing(0, 0, 0, 0),
-
-      [theme.breakpoints.up('md')]: {
-        margin: theme.spacing(2, 2, 0, 1),
-        padding: theme.spacing(3),
-      },
-    }),
-    canvasContent: css({
-      label: 'canvas-content',
-      display: 'flex',
-      flexDirection: 'column',
-      padding: theme.spacing(2),
-      flexBasis: '100%',
-      flexGrow: 1,
-    }),
-  };
-};

public/app/core/components/PageNew/PageContents.tsx

@@ -1,16 +0,0 @@
-// Libraries
-import React from 'react';
-
-import PageLoader from '../PageLoader/PageLoader';
-
-interface Props {
-  isLoading?: boolean;
-  children: React.ReactNode;
-  className?: string;
-}
-
-export const PageContents = ({ isLoading, children, className }: Props) => {
-  let content = className ? <div className={className}>{children}</div> : children;
-
-  return <>{isLoading ? <PageLoader /> : content}</>;
-};