TopNav: Section navigation UX (#55012)

* Design tweaks * Updated * Fixing unit tests * Review fixes * Text primary on active sections, and change home icon to text * spacing fix * More fix * Fixes * Updates
2025-02-25 18:55:37 -06:00 · 2022-09-13 12:24:23 +02:00 · 2022-09-13 12:24:23 +02:00 · da001b01f1
commit da001b01f1
parent 031c186617
14 changed files with 173 additions and 126 deletions
--- a/packages/grafana-ui/src/components/Tabs/VerticalTab.tsx
+++ b/packages/grafana-ui/src/components/Tabs/VerticalTab.tsx
@ -25,20 +25,18 @@ export const VerticalTab = React.forwardRef<HTMLAnchorElement, TabProps>(
    const linkClass = cx(tabsStyles.link, active && tabsStyles.activeStyle);

    return (
-      <div className={tabsStyles.item}>
-        <a
-          href={href}
-          className={linkClass}
-          {...otherProps}
-          onClick={onChangeTab}
-          aria-label={otherProps['aria-label'] || selectors.components.Tab.title(label)}
-          role="tab"
-          aria-selected={active}
-          ref={ref}
-        >
-          {content()}
-        </a>
-      </div>
+      <a
+        href={href}
+        className={linkClass}
+        {...otherProps}
+        onClick={onChangeTab}
+        aria-label={otherProps['aria-label'] || selectors.components.Tab.title(label)}
+        role="tab"
+        aria-selected={active}
+        ref={ref}
+      >
+        {content()}
+      </a>
    );
  }
 );
@ -47,17 +45,12 @@ VerticalTab.displayName = 'Tab';

 const getTabStyles = (theme: GrafanaTheme2) => {
  return {
-    item: css`
-      list-style: none;
-      margin-right: ${theme.spacing(2)};
-      position: relative;
-      display: block;
-    `,
    link: css`
      padding: 6px 12px;
      display: block;
      height: 100%;
      cursor: pointer;
+      position: relative;

      color: ${theme.colors.text.primary};

--- a/pkg/api/index.go
+++ b/pkg/api/index.go
@ -328,7 +328,8 @@ func (hs *HTTPServer) getNavTree(c *models.ReqContext, hasEditPerm bool, prefs *
 		// Move server admin into Configuration and rename to administration
 		if configNode != nil && serverAdminNode != nil {
 			configNode.Text = "Administration"
-			serverAdminNode.Url = "/admin"
+			serverAdminNode.Url = "/admin/server"
+			serverAdminNode.HideFromTabs = false
 			configNode.Children = append(configNode.Children, serverAdminNode)
 			adminNodeIndex := len(navTree) - 1
 			navTree = navTree[:adminNodeIndex]
--- a/public/app/core/components/AppChrome/NavToolbar.tsx
+++ b/public/app/core/components/AppChrome/NavToolbar.tsx
@ -59,6 +59,7 @@ const getStyles = (theme: GrafanaTheme2) => {
    menuButton: css({
      display: 'flex',
      alignItems: 'center',
+      marginRight: theme.spacing(1),
    }),
    actions: css({
      display: 'flex',
--- a/public/app/core/components/Breadcrumbs/BreadcrumbItem.tsx
+++ b/public/app/core/components/Breadcrumbs/BreadcrumbItem.tsx
@ -2,7 +2,7 @@ import { css, cx } from '@emotion/css';
 import React from 'react';

 import { GrafanaTheme2 } from '@grafana/data';
-import { Icon, LinkButton, useStyles2 } from '@grafana/ui';
+import { Icon, useStyles2 } from '@grafana/ui';

 import { Breadcrumb } from './types';

@ -20,20 +20,9 @@ export function BreadcrumbItem(props: Props) {
        </span>
      ) : (
        <>
-          {'icon' in props ? (
-            <LinkButton
-              size="md"
-              variant="secondary"
-              fill="text"
-              icon={props.icon}
-              href={props.href}
-              aria-label={props.text}
-            />
-          ) : (
-            <a className={cx(styles.breadcrumb, styles.breadcrumbLink)} href={props.href}>
-              {props.text}
-            </a>
-          )}
+          <a className={cx(styles.breadcrumb, styles.breadcrumbLink)} href={props.href}>
+            {props.text}
+          </a>
          <div className={styles.separator} aria-hidden={true}>
            <Icon name="angle-right" />
          </div>
--- a/public/app/core/components/Breadcrumbs/Breadcrumbs.test.tsx
+++ b/public/app/core/components/Breadcrumbs/Breadcrumbs.test.tsx
@ -5,7 +5,7 @@ import { Breadcrumbs } from './Breadcrumbs';
 import { Breadcrumb } from './types';

 const mockBreadcrumbs: Breadcrumb[] = [
-  { text: 'Home', href: '/home', icon: 'home-alt' },
+  { text: 'Home', href: '/home' },
  { text: 'First', href: '/first' },
  { text: 'Second', href: '/second' },
 ];
--- a/public/app/core/components/Breadcrumbs/utils.test.ts
+++ b/public/app/core/components/Breadcrumbs/utils.test.ts
@ -10,7 +10,7 @@ describe('breadcrumb utils', () => {
        url: '/my-section',
      };
      const result = buildBreadcrumbs(sectionNav);
-      expect(result[0]).toEqual({ icon: 'home-alt', href: '/', text: 'Home' });
+      expect(result[0]).toEqual({ href: '/', text: 'Home' });
    });

    it('includes breadcrumbs for the section nav', () => {
@ -19,7 +19,7 @@ describe('breadcrumb utils', () => {
        url: '/my-section',
      };
      expect(buildBreadcrumbs(sectionNav)).toEqual([
-        { icon: 'home-alt', href: '/', text: 'Home' },
+        { href: '/', text: 'Home' },
        { text: 'My section', href: '/my-section' },
      ]);
    });
@ -35,7 +35,7 @@ describe('breadcrumb utils', () => {
        url: '/my-page',
      };
      expect(buildBreadcrumbs(sectionNav, pageNav)).toEqual([
-        { icon: 'home-alt', href: '/', text: 'Home' },
+        { href: '/', text: 'Home' },
        { text: 'My section', href: '/my-section' },
        { text: 'My page', href: '/my-page' },
      ]);
@ -51,7 +51,7 @@ describe('breadcrumb utils', () => {
        },
      };
      expect(buildBreadcrumbs(sectionNav)).toEqual([
-        { icon: 'home-alt', href: '/', text: 'Home' },
+        { href: '/', text: 'Home' },
        { text: 'My parent section', href: '/my-parent-section' },
        { text: 'My section', href: '/my-section' },
      ]);
@ -75,7 +75,7 @@ describe('breadcrumb utils', () => {
        },
      };
      expect(buildBreadcrumbs(sectionNav, pageNav)).toEqual([
-        { icon: 'home-alt', href: '/', text: 'Home' },
+        { href: '/', text: 'Home' },
        { text: 'My parent section', href: '/my-parent-section' },
        { text: 'My section', href: '/my-section' },
        { text: 'My parent page', href: '/my-parent-page' },
--- a/public/app/core/components/Breadcrumbs/utils.ts
+++ b/public/app/core/components/Breadcrumbs/utils.ts
@ -3,7 +3,7 @@ import { NavModelItem } from '@grafana/data';
 import { Breadcrumb } from './types';

 export function buildBreadcrumbs(sectionNav: NavModelItem, pageNav?: NavModelItem) {
-  const crumbs: Breadcrumb[] = [{ icon: 'home-alt', href: '/', text: 'Home' }];
+  const crumbs: Breadcrumb[] = [{ href: '/', text: 'Home' }];

  function addCrumbs(node: NavModelItem) {
    if (node.parentItem) {
--- a/public/app/core/components/PageNew/Page.test.tsx
+++ b/public/app/core/components/PageNew/Page.test.tsx
@ -69,10 +69,10 @@ describe('Render', () => {
  it('should render section nav model based on navId', async () => {
    setup({ navId: 'child1' });

-    expect(screen.getByRole('heading', { name: 'Section name' })).toBeInTheDocument();
-    expect(screen.getByRole('heading', { name: 'Child1' })).toBeInTheDocument();
+    expect(screen.getByRole('tab', { name: 'Tab Section name' })).toBeInTheDocument();
    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
-    expect(screen.getAllByRole('tab').length).toBe(2);
+    expect(screen.getByRole('tab', { name: 'Tab Child1' })).toBeInTheDocument();
+    expect(screen.getAllByRole('tab').length).toBe(3);
  });

  it('should update chrome with section and pageNav', async () => {
@ -84,7 +84,7 @@ describe('Render', () => {
  it('should render section nav model based on navId and item page nav', async () => {
    setup({ navId: 'child1', pageNav });

-    expect(screen.getByRole('heading', { name: 'Section name' })).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();
--- a/public/app/core/components/PageNew/SectionNav.tsx
+++ b/public/app/core/components/PageNew/SectionNav.tsx
@ -2,55 +2,22 @@ import { css } from '@emotion/css';
 import React from 'react';

 import { NavModel, GrafanaTheme2 } from '@grafana/data';
-import { useStyles2, Icon, VerticalTab, toIconName, CustomScrollbar } from '@grafana/ui';
+import { useStyles2, CustomScrollbar } from '@grafana/ui';
+
+import { SectionNavItem } from './SectionNavItem';

 export interface Props {
  model: NavModel;
 }

-export function SectionNav(props: Props) {
+export function SectionNav({ model }: Props) {
  const styles = useStyles2(getStyles);

-  const main = props.model.main;
-  const directChildren = props.model.main.children!.filter((x) => !x.hideFromTabs && !x.children);
-  const nestedItems = props.model.main.children!.filter((x) => x.children && x.children.length);
-  const icon = main.icon ? toIconName(main.icon) : undefined;
-
  return (
    <nav className={styles.nav}>
-      <h2 className={styles.sectionName}>
-        {icon && <Icon name={icon} size="lg" />}
-        {main.img && <img className={styles.sectionImg} src={main.img} alt={`logo of ${main.text}`} />}
-        {props.model.main.text}
-      </h2>
      <CustomScrollbar showScrollIndicators>
        <div className={styles.items} role="tablist">
-          {directChildren.map((child, index) => {
-            return (
-              !child.hideFromTabs &&
-              !child.children && (
-                <VerticalTab label={child.text} active={child.active} key={`${child.url}-${index}`} href={child.url} />
-              )
-            );
-          })}
-          {nestedItems.map((child) => (
-            <>
-              <div className={styles.subSection}>{child.text}</div>
-              {child.children!.map((child, index) => {
-                return (
-                  !child.hideFromTabs &&
-                  !child.children && (
-                    <VerticalTab
-                      label={child.text}
-                      active={child.active}
-                      key={`${child.url}-${index}`}
-                      href={child.url}
-                    />
-                  )
-                );
-              })}
-            </>
-          ))}
+          <SectionNavItem item={model.main} />
        </div>
      </CustomScrollbar>
    </nav>
@ -63,27 +30,15 @@ const getStyles = (theme: GrafanaTheme2) => {
      display: 'flex',
      flexDirection: 'column',
      background: theme.colors.background.canvas,
-      padding: theme.spacing(3, 2),
      flexShrink: 0,
      [theme.breakpoints.up('md')]: {
        width: '250px',
      },
    }),
-    sectionName: css({
+    items: css({
      display: 'flex',
-      alignItems: 'center',
-      gap: theme.spacing(1),
-      padding: theme.spacing(0.5, 0, 3, 0.25),
-      fontSize: theme.typography.h4.fontSize,
-      margin: 0,
-    }),
-    items: css({}),
-    sectionImg: css({
-      height: 48,
-    }),
-    subSection: css({
-      padding: theme.spacing(3, 0, 0.5, 1),
-      fontWeight: 500,
+      flexDirection: 'column',
+      padding: theme.spacing(4.5, 1, 2, 2),
    }),
  };
 };
--- a/public/app/core/components/PageNew/SectionNavItem.tsx
+++ b/public/app/core/components/PageNew/SectionNavItem.tsx
@ -0,0 +1,112 @@
+import { css, cx } from '@emotion/css';
+import React from 'react';
+
+import { GrafanaTheme2, NavModelItem } from '@grafana/data';
+import { selectors } from '@grafana/e2e-selectors';
+import { useStyles2, Icon } from '@grafana/ui';
+
+export interface Props {
+  item: NavModelItem;
+}
+
+export function SectionNavItem({ item }: Props) {
+  const styles = useStyles2(getStyles);
+
+  const children = item.children?.filter((x) => !x.hideFromTabs);
+  const isRoot = item.parentItem == null;
+  const hasActiveChild = Boolean(children?.length && children.find((x) => x.active));
+
+  // If first root child is a section skip the bottom margin (as sections have top margin already)
+  const noRootMargin = isRoot && Boolean(item.children![0].children?.length);
+
+  const linkClass = cx({
+    [styles.link]: true,
+    [styles.activeStyle]: item.active,
+    [styles.isSection]: Boolean(children?.length),
+    [styles.hasActiveChild]: hasActiveChild,
+    [styles.isRoot]: isRoot,
+    [styles.noRootMargin]: noRootMargin,
+  });
+
+  return (
+    <>
+      <a
+        href={item.url}
+        className={linkClass}
+        aria-label={selectors.components.Tab.title(item.text)}
+        role="tab"
+        aria-selected={item.active}
+      >
+        {isRoot && item.icon && <Icon name={item.icon} />}
+        {isRoot && item.img && <img className={styles.sectionImg} src={item.img} alt={`logo of ${item.text}`} />}
+        {item.text}
+        {item.tabSuffix && <item.tabSuffix className={styles.suffix} />}
+      </a>
+      {children?.map((child, index) => (
+        <SectionNavItem item={child} key={index} />
+      ))}
+    </>
+  );
+}
+
+const getStyles = (theme: GrafanaTheme2) => {
+  return {
+    link: css`
+      padding: ${theme.spacing(1, 0, 1, 1.5)};
+      display: flex;
+      align-items: center;
+      gap: ${theme.spacing(1)};
+      height: 100%;
+      position: relative;
+      color: ${theme.colors.text.secondary};
+
+      &:hover,
+      &:focus {
+        text-decoration: underline;
+      }
+    `,
+    activeStyle: css`
+      label: activeTabStyle;
+      color: ${theme.colors.text.primary};
+      background-color: ${theme.colors.action.disabledBackground};
+      border-radius: ${theme.shape.borderRadius(2)};
+      fontweight: theme.typography.fontWeightMedium;
+
+      &::before {
+        display: block;
+        content: ' ';
+        position: absolute;
+        left: 0;
+        width: 4px;
+        bottom: 2px;
+        top: 2px;
+        border-radius: 2px;
+        background-image: ${theme.colors.gradients.brandVertical};
+      }
+    `,
+    suffix: css`
+      margin-left: ${theme.spacing(1)};
+    `,
+    sectionImg: css({
+      height: 18,
+    }),
+    isRoot: css({
+      color: theme.colors.text.primary,
+      fontSize: theme.typography.h4.fontSize,
+      marginTop: 0,
+      marginBottom: theme.spacing(2),
+      fontWeight: theme.typography.fontWeightMedium,
+    }),
+    isSection: css({
+      fontSize: theme.typography.h5.fontSize,
+      marginTop: theme.spacing(2),
+      fontWeight: theme.typography.fontWeightMedium,
+    }),
+    noRootMargin: css({
+      marginBottom: 0,
+    }),
+    hasActiveChild: css({
+      color: theme.colors.text.primary,
+    }),
+  };
+};
--- a/public/app/core/selectors/navModel.test.ts
+++ b/public/app/core/selectors/navModel.test.ts
@ -46,7 +46,7 @@ describe('getNavModel', () => {
    const navModel = getNavModel(navIndex, 'apps/subapp/child1');
    expect(navModel.main.id).toBe('apps');
    expect(navModel.node.id).toBe('apps/subapp/child1');
-    expect(navModel.main.children![2].active).toBe(true);
+    expect(navModel.main.children![2].active).toBe(undefined);
    expect(navModel.main.children![2].children![0].active).toBe(true);
  });
 });
--- a/public/app/core/selectors/navModel.ts
+++ b/public/app/core/selectors/navModel.ts
@ -18,12 +18,12 @@ const getNotFoundModel = (): NavModel => {
 export const getNavModel = (navIndex: NavIndex, id: string, fallback?: NavModel, onlyChild = false): NavModel => {
  if (navIndex[id]) {
    const node = navIndex[id];
-    const nodeWithActive = enrichNodeWithActiveState(node);
-    const main = onlyChild ? nodeWithActive : getSectionRoot(nodeWithActive);
+    const main = onlyChild ? node : getSectionRoot(node);
+    const mainWithActive = enrichNodeWithActiveState(main, id);

    return {
-      node: nodeWithActive,
-      main,
+      node: node,
+      main: mainWithActive,
    };
  }

@ -38,27 +38,19 @@ function getSectionRoot(node: NavModelItem): NavModelItem {
  return node.parentItem ? getSectionRoot(node.parentItem) : node;
 }

-function enrichNodeWithActiveState(node: NavModelItem): NavModelItem {
-  const nodeCopy = { ...node };
-
-  if (nodeCopy.parentItem) {
-    nodeCopy.parentItem = { ...nodeCopy.parentItem };
-    const root = nodeCopy.parentItem;
-
-    if (root.children) {
-      root.children = root.children.map((item) => {
-        if (item.id === node.id) {
-          return { ...nodeCopy, active: true };
-        }
-
-        return item;
-      });
-    }
-
-    nodeCopy.parentItem = enrichNodeWithActiveState(root);
+function enrichNodeWithActiveState(node: NavModelItem, activeId: string): NavModelItem {
+  if (node.id === activeId) {
+    return { ...node, active: true };
  }

-  return nodeCopy;
+  if (node.children && node.children.length > 0) {
+    return {
+      ...node,
+      children: node.children.map((child) => enrichNodeWithActiveState(child, activeId)),
+    };
+  }
+
+  return node;
 }

 export const getTitleFromNavModel = (navModel: NavModel) => {
--- a/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.test.tsx
+++ b/public/app/features/dashboard/components/DashboardSettings/DashboardSettings.test.tsx
@ -51,6 +51,6 @@ describe('DashboardSettings', () => {
      </GrafanaContext.Provider>
    );

-    expect(await screen.findByRole('heading', { name: 'Settings' })).toBeInTheDocument();
+    expect(await screen.findByRole('tab', { name: 'Tab Settings' })).toBeInTheDocument();
  });
 });
--- a/public/app/routes/routes.tsx
+++ b/public/app/routes/routes.tsx
@ -298,11 +298,15 @@ export function getAppRoutes(): RouteDescriptor[] {
      component: SafeDynamicImport(() => import(/* webpackChunkName: "TeamPages" */ 'app/features/teams/TeamPages')),
    },
    // ADMIN
-
    {
      path: '/admin',
      component: () => (config.featureToggles.topnav ? <NavLandingPage navId="cfg" /> : <Redirect to="/admin/users" />),
    },
+    {
+      path: '/admin/server',
+      component: () =>
+        config.featureToggles.topnav ? <NavLandingPage navId="admin" /> : <Redirect to="/admin/users" />,
+    },
    {
      path: '/admin/settings',
      component: SafeDynamicImport(