improve notification badge ux

graylog2-web-interface/src/@types/graylog-web-plugin/index.d.ts

@@ -47,7 +47,7 @@ interface GlobalNotification {
 interface NavigationBadge {
   key: string;
   component: React.ComponentType;
-  // Active badges replace the built-in notification badge, so they must render the notification count themselves.
+  // Active badges render alongside the built-in notification badge, so they must not duplicate its count.
   useCondition: () => boolean;
 }
 

graylog2-web-interface/src/components/bootstrap/Badge.tsx

@@ -16,6 +16,7 @@
  */
 import * as React from 'react';
 import type { ColorVariant } from '@graylog/sawmill';
+import type { BadgeVariant } from '@mantine/core';
 import { Badge as MantineBadge } from '@mantine/core';
 import styled, { css, useTheme } from 'styled-components';
 import type { DefaultTheme } from 'styled-components';
@@ -34,11 +35,19 @@ const mapFontSize: Record<SupportedMantineSize, 'tiny' | 'small' | 'body'> = {
   lg: 'body',
 };
 
-const StyledBadge = styled(MantineBadge)<{ color: ColorVariant; size: SupportedMantineSize }>(
-  ({ theme, color, size }) => css`
+const StyledBadge = styled(MantineBadge)<{
+  color: ColorVariant;
+  size: SupportedMantineSize;
+  variant: BadgeVariant;
+}>(
+  ({ theme, color, size, variant }) => css`
     text-transform: none;
-    background-color: ${color};
-    color: ${theme.utils.contrastingColor(color)};
+    ${variant === 'filled'
+      ? css`
+          background-color: ${color};
+          color: ${theme.utils.contrastingColor(color)};
+        `
+      : ''}
 
     /* Let the badge shrink below its content width — as a flex/grid item (min-width: 0) and
        capped to its container (max-width: 100%) instead of Mantine's default width: fit-content.
@@ -74,6 +83,7 @@ type Props = React.PropsWithChildren<{
   style?: React.CSSProperties;
   title?: string;
   uppercase?: boolean;
+  variant?: BadgeVariant;
 }>;
 
 const Badge = (
@@ -91,6 +101,7 @@ const Badge = (
     title = undefined,
     bsSize = 'md',
     uppercase = false,
+    variant = 'filled',
   }: Props,
   ref: React.ForwardedRef<HTMLElement>,
 ) => {
@@ -106,7 +117,7 @@ const Badge = (
     'data-testid': dataTestid,
     role,
     style,
-    variant: 'filled' as const,
+    variant,
     onMouseEnter,
     onMouseLeave,
     size,

graylog2-web-interface/src/components/navigation/NavBadgeItem.tsx

@@ -0,0 +1,57 @@
+/*
+ * Copyright (C) 2020 Graylog, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the Server Side Public License, version 1,
+ * as published by MongoDB, Inc.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * Server Side Public License for more details.
+ *
+ * You should have received a copy of the Server Side Public License
+ * along with this program. If not, see
+ * <http://www.mongodb.com/licensing/server-side-public-license>.
+ */
+import * as React from 'react';
+import styled from 'styled-components';
+
+import { LinkContainer } from 'components/common';
+import { Nav } from 'components/bootstrap';
+import { NAV_ITEM_HEIGHT } from 'theme/constants';
+
+import InactiveNavItem from './InactiveNavItem';
+
+const StyledNav = styled(Nav)`
+  > li > a {
+    min-height: ${NAV_ITEM_HEIGHT};
+    display: inline-flex;
+    align-items: center;
+    justify-content: center;
+    padding: 12px;
+    cursor: pointer;
+  }
+`;
+
+const StyledInactiveNavItem = styled(InactiveNavItem)`
+  a:hover {
+    border: 0;
+    text-decoration: none;
+  }
+`;
+
+type Props = React.PropsWithChildren<{
+  onClick?: () => void;
+  to: string;
+}>;
+
+const NavBadgeItem = ({ children = undefined, onClick = undefined, to }: Props) => (
+  <StyledNav navbar>
+    <LinkContainer to={to} onClick={onClick}>
+      <StyledInactiveNavItem>{children}</StyledInactiveNavItem>
+    </LinkContainer>
+  </StyledNav>
+);
+
+export default NavBadgeItem;

graylog2-web-interface/src/components/navigation/Navigation.test.tsx

@@ -70,17 +70,15 @@ describe('Navigation', () => {
     await screen.findByTestId('notification-badge');
   });
 
-  it('does not show notification badge when there are no notifications', async () => {
+  it('shows notification badge without a count when there are no notifications', async () => {
     asMock(useNotificationBadgeCount).mockReturnValue({
       data: 0,
       isLoading: false,
     });
 
     render(<SUT />);
 
-    await screen.findByRole('button', { name: /help/i });
-
-    expect(screen.queryByTestId('notification-badge')).not.toBeInTheDocument();
+    expect(await screen.findByTestId('notification-badge')).toHaveAccessibleName('No unread system notifications');
   });
 
   describe('with a plugin navigation badge', () => {
@@ -104,18 +102,18 @@ describe('Navigation', () => {
       PluginStore.unregister(plugin);
     });
 
-    it('replaces the notification badge when the plugin badge is active', async () => {
+    it('shows the notification badge next to an active plugin badge', async () => {
       plugin = badgePlugin(() => true);
       PluginStore.register(plugin);
 
       render(<SUT />);
 
       await screen.findByTestId('plugin-badge');
 
-      expect(screen.queryByTestId('notification-badge')).not.toBeInTheDocument();
+      expect(screen.getByTestId('notification-badge')).toBeInTheDocument();
     });
 
-    it('falls back to the notification badge when the plugin badge is inactive', async () => {
+    it('still shows the notification badge when the plugin badge is inactive', async () => {
       plugin = badgePlugin(() => false);
       PluginStore.register(plugin);
 

graylog2-web-interface/src/components/navigation/Navigation.tsx

@@ -85,7 +85,7 @@ const Navigation = React.memo(({ pathname }: Props) => {
         {activePluginBadges.map(({ key, component: PluginBadge }) => (
           <PluginBadge key={key} />
         ))}
-        {activePluginBadges.length === 0 && <NotificationBadge />}
+        <NotificationBadge />
 
         <Nav pullRight className="header-meta-nav">
           {AppConfig.isFeatureEnabled(FEATURE_FLAG) ? <QuickJumpModalContainer /> : null}

graylog2-web-interface/src/components/navigation/NotificationBadge.test.tsx

@@ -16,7 +16,7 @@
  */
 import * as React from 'react';
 import Immutable from 'immutable';
-import { render, screen, waitFor, within } from 'wrappedTestingLibrary';
+import { render, screen, waitFor } from 'wrappedTestingLibrary';
 
 import { asMock } from 'helpers/mocking';
 import { adminUser } from 'fixtures/users';
@@ -39,6 +39,7 @@ const setBadgeCount = (count: number) =>
 describe('NotificationBadge', () => {
   beforeEach(() => {
     asMock(useCurrentUser).mockReturnValue(adminUser);
+    setBadgeCount(0);
   });
 
   it('renders nothing when user has no notification permissions', () => {
@@ -47,20 +48,37 @@ describe('NotificationBadge', () => {
       .permissions(Immutable.List(['dashboards:read']))
       .build();
     asMock(useCurrentUser).mockReturnValue(userWithoutPermissions);
-    asMock(useNotificationBadgeCount).mockReturnValue({ data: undefined, isLoading: false } as never);
 
     render(<NotificationBadge />);
 
     expect(useNotificationBadgeCount).toHaveBeenCalledWith({ enabled: false });
     expect(screen.queryByTestId(BADGE_ID)).not.toBeInTheDocument();
   });
 
-  it('renders nothing while loading', () => {
-    asMock(useNotificationBadgeCount).mockReturnValue({ data: undefined, isLoading: true } as never);
+  it('links to the system notifications page', async () => {
+    render(<NotificationBadge />);
+
+    expect(await screen.findByRole('link', { name: 'No unread system notifications' })).toHaveAttribute(
+      'href',
+      '/system/notifications',
+    );
+  });
 
+  it('shows the icon without a count when there are no unread notifications', async () => {
     render(<NotificationBadge />);
 
-    expect(screen.queryByTestId(BADGE_ID)).not.toBeInTheDocument();
+    const badge = await screen.findByTestId(BADGE_ID);
+
+    expect(badge).toHaveAccessibleName('No unread system notifications');
+    expect(badge).not.toHaveTextContent('0');
+  });
+
+  it('shows no count while loading', async () => {
+    asMock(useNotificationBadgeCount).mockReturnValue({ data: 0, isLoading: true });
+
+    render(<NotificationBadge />);
+
+    expect(await screen.findByTestId(BADGE_ID)).toHaveAccessibleName('No unread system notifications');
   });
 
   it('renders count when there are unread notifications', async () => {
@@ -70,26 +88,42 @@ describe('NotificationBadge', () => {
 
     const badge = await screen.findByTestId(BADGE_ID);
 
-    expect(within(badge).getByText(42)).toBeInTheDocument();
+    expect(badge).toHaveTextContent('42');
+    expect(badge).toHaveAccessibleName('42 unread system notifications');
+  });
+
+  it('uses a singular accessible name for a single notification', async () => {
+    setBadgeCount(1);
+
+    render(<NotificationBadge />);
+
+    expect(await screen.findByTestId(BADGE_ID)).toHaveAccessibleName('1 unread system notification');
+  });
+
+  it('caps the displayed count', async () => {
+    setBadgeCount(120);
+
+    render(<NotificationBadge />);
+
+    const badge = await screen.findByTestId(BADGE_ID);
+
+    expect(badge).toHaveTextContent('99+');
+    expect(badge).toHaveAccessibleName('120 unread system notifications');
   });
 
   it('updates the badge count on subsequent polls', async () => {
     setBadgeCount(42);
 
     const { rerender } = render(<NotificationBadge />);
 
-    const badgeBefore = await screen.findByTestId(BADGE_ID);
-
-    expect(within(badgeBefore).getByText(42)).toBeInTheDocument();
+    expect(await screen.findByTestId(BADGE_ID)).toHaveTextContent('42');
 
     setBadgeCount(23);
 
     rerender(<NotificationBadge />);
 
-    const badgeAfter = await screen.findByTestId(BADGE_ID);
-
     await waitFor(() => {
-      expect(within(badgeAfter).getByText(23)).toBeInTheDocument();
+      expect(screen.getByTestId(BADGE_ID)).toHaveTextContent('23');
     });
   });
 });

graylog2-web-interface/src/components/navigation/NotificationBadge.tsx

@@ -15,49 +15,40 @@
  * <http://www.mongodb.com/licensing/server-side-public-license>.
  */
 import * as React from 'react';
-import styled from 'styled-components';
 
-import { LinkContainer } from 'components/common';
-import { Badge, Nav } from 'components/bootstrap';
+import { Group, Icon } from 'components/common';
+import { Badge } from 'components/bootstrap';
 import usePermissions from 'hooks/usePermissions';
 import useNotificationBadgeCount from 'components/notifications/hooks/useNotificationBadgeCount';
 import Routes from 'routing/Routes';
-import { NAV_ITEM_HEIGHT } from 'theme/constants';
+import StringUtils from 'util/StringUtils';
 
-import InactiveNavItem from './InactiveNavItem';
+import NavBadgeItem from './NavBadgeItem';
 
-const StyledNav = styled(Nav)`
-  > li > a {
-    min-height: ${NAV_ITEM_HEIGHT};
-    display: inline-flex;
-    align-items: center;
-    justify-content: center;
-    padding: 12px;
-  }
-`;
-
-const StyledInactiveNavItem = styled(InactiveNavItem)`
-  a:hover {
-    border: 0;
-    text-decoration: none;
-  }
-`;
+const MAX_DISPLAYED_COUNT = 99;
 
 const NotificationBadge = () => {
   const { isPermitted } = usePermissions();
   const enabled = isPermitted('notifications:read');
-  const { data, isLoading } = useNotificationBadgeCount({ enabled });
+  const { data: count } = useNotificationBadgeCount({ enabled });
+
+  if (!enabled) return null;
+
+  const accessibleLabel =
+    count > 0
+      ? `${count} unread system ${StringUtils.pluralize(count, 'notification', 'notifications')}`
+      : 'No unread system notifications';
+  const displayedCount = count > MAX_DISPLAYED_COUNT ? `${MAX_DISPLAYED_COUNT}+` : count;
 
-  return isLoading || !data ? null : (
-    <StyledNav navbar>
-      <LinkContainer to={Routes.SYSTEM.NOTIFICATIONS}>
-        <StyledInactiveNavItem>
-          <Badge bsStyle="danger" data-testid="notification-badge" title="System Notifications">
-            {data}
-          </Badge>
-        </StyledInactiveNavItem>
-      </LinkContainer>
-    </StyledNav>
+  return (
+    <NavBadgeItem to={Routes.SYSTEM.NOTIFICATIONS}>
+      <Badge aria-label={accessibleLabel} data-testid="notification-badge" title={accessibleLabel} variant="default">
+        <Group component="span" gap={4} wrap="nowrap">
+          <Icon name="notifications" size="sm" />
+          {count > 0 ? displayedCount : null}
+        </Group>
+      </Badge>
+    </NavBadgeItem>
   );
 };