diff --git a/app/src/components/Theme.tsx b/app/src/components/Theme.tsx index 3c5d6625..7e9b0aa8 100644 --- a/app/src/components/Theme.tsx +++ b/app/src/components/Theme.tsx @@ -215,7 +215,12 @@ export function ThemeProvider({children, ...props}: ThemeProviderProps) { cssVariablesResolver={cssVariablesResolver} forceColorScheme={dark ? 'dark' : 'light'} > - + {/* + * Top right. Bottom right put toasts over the sticky footer on the file browser + * and over the player controls on a video, which are the two places a background + * task is most likely to report while the user is doing something else. + */} + {children} diff --git a/app/src/components/ThemeSamplePage.js b/app/src/components/ThemeSamplePage.js index 730186e4..7d2dabc6 100644 --- a/app/src/components/ThemeSamplePage.js +++ b/app/src/components/ThemeSamplePage.js @@ -33,6 +33,7 @@ import { ThemePicker, Toggle, Tooltip, + clearToasts, toast, } from './ui'; import {semanticColorNames} from '../themes/mantine'; @@ -61,6 +62,10 @@ const Row = ({children}) =>
; +// Numbers the stacked toasts so it is obvious which press produced which, and that a second +// press adds a toast rather than replacing the first. Module scope, so it survives re-renders. +let toastCounter = 0; + export function ThemeSamplePage() { const {theme} = useContext(ThemeContext); const [confirmOpen, setConfirmOpen] = useState(false); @@ -212,6 +217,77 @@ export function ThemeSamplePage() { +
+ + {/* + * `toast` has thirteen call sites and no test, and it has already failed once + * app-wide: App.js dropped Semantic's toast container while seven pages were + * still importing the old helper, and every notification silently vanished. + * A toast is also the one component you cannot see by loading a page -- it has + * to be provoked -- which is why it needs buttons here rather than a sample. + */} + + + + + + + + + {/* Click this repeatedly: each press is a separate toast, and the + container holds five at once. */} + + + + + + + + +

+ Bottom right, five at a time, five seconds each unless the caller says + otherwise. Check each type in all four themes: night has no second hue to + spend, so info, success, warning and error cannot be told apart by colour + there. +

+ +
+
diff --git a/app/src/components/ui/toast.test.js b/app/src/components/ui/toast.test.js new file mode 100644 index 00000000..6e94768a --- /dev/null +++ b/app/src/components/ui/toast.test.js @@ -0,0 +1,204 @@ +import React from 'react'; +import {act, render, screen, waitFor} from '@testing-library/react'; +import {MantineProvider} from '@mantine/core'; +import {Notifications} from '@mantine/notifications'; +import {cssVariablesResolver, mantineTheme} from '../../themes/mantine'; +import {clearToasts, toast} from './toast'; + +/* + * Tests for toasts. + * + * `toast` has thirteen call sites and had no test at all, having already failed once in the + * way that is hardest to notice: App.js dropped Semantic's toast container while seven pages + * were still importing the old helper, so every notification in the app silently went + * nowhere. Nothing threw, and nothing looked wrong until somebody expected a message. + * + * The container is mounted here the way ThemeProvider mounts it, because a toast with no + * container is exactly the failure worth covering. + */ + +const withToasts = () => render( + + + +); + +const showToast = (options) => act(() => { + toast(options); +}); + +const visibleToasts = () => + document.querySelectorAll('[class*="mantine-Notification-root"]'); + +afterEach(() => { + act(() => clearToasts()); +}); + +describe('toast', () => { + it('shows a title and its description', async () => { + withToasts(); + + showToast({type: 'success', title: 'Saved', description: 'Channel settings written.'}); + + expect(await screen.findByText('Saved')).toBeInTheDocument(); + expect(screen.getByText('Channel settings written.')).toBeInTheDocument(); + }); + + it('shows a description on its own', async () => { + // Several call sites send no title. + withToasts(); + + showToast({description: 'Nothing to download.'}); + + expect(await screen.findByText('Nothing to download.')).toBeInTheDocument(); + }); + + it('stacks, rather than replacing what is already up', async () => { + /* + * The behaviour that matters when a background task reports repeatedly -- three + * downloads finishing should be three messages, not one that keeps being overwritten. + * Mantine replaces a notification when it is given an `id` it has already seen, so + * this would break the moment somebody added one to dedupe. + */ + withToasts(); + + showToast({title: 'First'}); + showToast({title: 'Second'}); + showToast({title: 'Third'}); + + await waitFor(() => expect(visibleToasts()).toHaveLength(3)); + expect(screen.getByText('First')).toBeInTheDocument(); + expect(screen.getByText('Third')).toBeInTheDocument(); + }); + + it('stacks two identical toasts as two', async () => { + // Same title and description twice over: still two messages. + withToasts(); + + showToast({title: 'Download failed', description: 'HTTP 403'}); + showToast({title: 'Download failed', description: 'HTTP 403'}); + + await waitFor(() => expect(visibleToasts()).toHaveLength(2)); + }); + + it('holds no more than the container allows', async () => { + // Five at once, so the sixth waits rather than the stack growing without limit. + withToasts(); + + for (let i = 1; i <= 7; i += 1) { + showToast({title: `Toast ${i}`}); + } + + await waitFor(() => expect(visibleToasts()).toHaveLength(5)); + }); + + it('clears everything on request', async () => { + // Used when navigating away from a failed action, so its errors do not follow you. + withToasts(); + showToast({title: 'One'}); + showToast({title: 'Two'}); + await waitFor(() => expect(visibleToasts()).toHaveLength(2)); + + act(() => clearToasts()); + + await waitFor(() => expect(visibleToasts()).toHaveLength(0)); + }); + + it('carries the colour its type maps to', async () => { + withToasts(); + + showToast({type: 'error', title: 'Download failed'}); + + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + // Mantine writes the resolved colour onto the element as a custom property. + const notification = visibleToasts()[0]; + expect(notification.getAttribute('style')).toContain('red'); + }); + + it('gives each type a colour of its own', async () => { + /* + * Four types that must not collapse into one in light and dark. Night and amber are + * the deliberate exception -- they have no second hue to spend -- but that is decided + * by the token tables, not here, and here they have to differ. + */ + withToasts(); + + const colours = new Set(); + for (const type of ['info', 'success', 'warning', 'error']) { + showToast({type, title: `A ${type}`}); + } + await waitFor(() => expect(visibleToasts()).toHaveLength(4)); + for (const notification of visibleToasts()) { + colours.add((notification.getAttribute('style') || '').match(/--notification-color:[^;]*/)?.[0]); + } + + expect(colours.size).toBe(4); + }); + + it('defaults to info when given no type', async () => { + withToasts(); + + showToast({title: 'Refresh started'}); + + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + expect(visibleToasts()[0].getAttribute('style')).toContain('blue'); + }); + + it('survives being called with nothing at all', () => { + // Defensive: a call site building options from an API response can end up with none. + withToasts(); + + expect(() => showToast()).not.toThrow(); + }); +}); + +describe('how long a toast stays up', () => { + /* + * Real timers, with short durations chosen by the caller. + * + * Fake timers do not work here. Hiding a notification runs an exit transition -- a timer + * plus a state update plus a transition end -- and advancing the clock, even awaited inside + * `act`, never gets the element removed from the DOM. Both the assertion and its control + * then read the same whatever the code does, which is a test that cannot fail. It read + * green against a deliberately planted `autoClose: 0` before this was rewritten. + */ + + it('goes away on its own', async () => { + withToasts(); + showToast({title: 'Saved', time: 60}); + + await waitFor(() => expect(screen.queryByText('Saved')).not.toBeInTheDocument()); + }); + + it('is still up a moment after being shown, when no time is given', async () => { + // The default is five seconds, which is not worth waiting for; what is worth asserting + // is that a caller who names no time does not get a toast that vanishes instantly. + withToasts(); + showToast({title: 'Refresh started'}); + + await new Promise(resolve => setTimeout(resolve, 150)); + + expect(screen.getByText('Refresh started')).toBeInTheDocument(); + }); + + it('stays until dismissed when the caller asks for time 0', async () => { + /* + * Semantic treated `time: 0` as "stay up". Mantine wants `autoClose: false`: its + * `getAutoClose` returns any number as given, and the container then calls + * `setTimeout(hide, 0)` -- so passing the 0 straight through dismisses the toast on the + * next tick. Call sites use 0 for errors the user must actually see, so getting this + * backwards would throw away precisely the messages that matter most. + * + * A toast that expires normally goes up alongside it as a control. Waiting for that one + * to disappear proves dismissal is working, which is what makes the survivor a fact + * about `time: 0` rather than about how long the test happened to wait. + */ + withToasts(); + showToast({title: 'Expires normally', time: 60}); + showToast({title: 'Download failed', time: 0}); + + await waitFor(() => expect(screen.queryByText('Expires normally')).not.toBeInTheDocument()); + + expect(screen.getByText('Download failed')).toBeInTheDocument(); + }); +}); diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index 16406f9b..6e1a6f8b 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -779,6 +779,54 @@ html[data-theme="amber"] .wrolpi-label { --label-text: var(--btn-text) !important; } +/* ----------------------------------------------------------------- Toasts */ +/* + * Notifications take their colours from our tokens rather than from Mantine's colour scheme. + * + * Mantine paints a dark-scheme notification title with `--mantine-color-white`, and the + * bridge aliases that to `--btn-text` -- deliberately, because it is the text drawn on a + * filled button, which is near-black in dark, night and amber. So the title came out at + * rgb(28, 31, 33) on a rgb(38, 41, 43) surface: about 1.1:1, present and invisible. Fixing + * the alias is not an option; it is correct for the buttons it exists for. + * + * The `html` prefix is only there to outrank Mantine's single-class rules. One set of values + * covers every theme, because the tokens are what differ. + */ + +html .mantine-Notification-root { + background-color: var(--panel); + border-color: var(--border); +} + +html .mantine-Notification-title { + color: var(--text); +} + +html .mantine-Notification-description { + color: var(--muted); +} + +/* + * The description is the message itself, so it has to be read, and `--muted` is too dim to + * read on a panel in the two monochrome themes -- 2.0:1 in night, 3.1:1 in amber, against + * 5.1:1 and 5.7:1 in dark and light. Those two drop the muting and let the title's weight + * carry the hierarchy instead, which is the same brightness-over-hue trade the rest of night + * makes. + */ +html[data-theme="night"] .mantine-Notification-description, +html[data-theme="amber"] .mantine-Notification-description { + color: var(--text); +} + +html .mantine-Notification-closeButton { + color: var(--muted); +} + +html .mantine-Notification-closeButton:hover { + background-color: var(--head); + color: var(--text); +} + /* --------------------------------------------------------------- Messages */ .wrolpi-message {