diff --git a/app/src/components/ThemeSamplePage.js b/app/src/components/ThemeSamplePage.js index 13138b33..2b8d0f5b 100644 --- a/app/src/components/ThemeSamplePage.js +++ b/app/src/components/ThemeSamplePage.js @@ -313,6 +313,61 @@ export function ThemeSamplePage() { +
+ + {/* + The roles side by side, which is the only way to see the thing that + matters: in night and amber these are five brightnesses of one hue, and + they have to stay tellable apart with no hue to help. + */} + {/* 120px so all five usually sit on one row; they still wrap on a phone, + which is why the caption says "in order" and not "left to right". */} +
+ {[ + ['neutral', 'pending, disabled, unknown'], + ['info', 'in progress, informational'], + ['success', 'complete, healthy'], + ['warning', 'needs attention'], + ['danger', 'failed, destructive'], + ].map(([role, meaning]) =>
+
+
+ {role} +
+
{meaning}
+
)} +
+

+ Components ask for a meaning — danger — not a hue. + Light and dark spend a colour on each. Night and amber have only one hue, + so a role is a step on a brightness ramp instead; that is why an error and + an info toast used to be the same pixel there. Severity climbs in the order + shown for those two themes, and danger is never quieter than + ordinary text. +

+ +
+ +
+ +
+ complete + downloading + pending + failed +
+

+ Four states drawn from four roles. Failure also carries weight, because + brightness is the first thing lost on a dim screen and it is all night has. +

+ +
+
diff --git a/app/src/components/ui/Feedback.tsx b/app/src/components/ui/Feedback.tsx index 1b91faf3..8c44797e 100644 --- a/app/src/components/ui/Feedback.tsx +++ b/app/src/components/ui/Feedback.tsx @@ -1,7 +1,7 @@ import React from 'react'; import {Loader as MLoader, Skeleton} from '@mantine/core'; import {Icon} from './Icon'; -import {SemanticColorName} from '../../themes/mantine'; +import {RoleName, SemanticColorName} from '../../themes/mantine'; /* * Feedback: messages, labels, progress, status text, loaders. @@ -12,13 +12,20 @@ import {SemanticColorName} from '../../themes/mantine'; // ------------------------------------------------------------------ Messages -export type MessageKind = 'info' | 'success' | 'warning' | 'error'; +/* + * `error` is the original spelling and stays: 26 call sites use it, and renaming those + * is app-level churn for no behaviour change. `danger` is the same thing under the + * name the token uses, and is what new code should write. + */ +export type MessageKind = 'info' | 'success' | 'warning' | 'error' | 'danger'; -const messageColors: Record = { - info: 'var(--blue)', - success: 'var(--green)', - warning: 'var(--amber)', - error: 'var(--red)', +/** The role each kind means. Never a hue -- see the note on roles in tokens.css. */ +const messageRoles: Record = { + info: 'info', + success: 'success', + warning: 'warning', + error: 'danger', + danger: 'danger', }; export interface MessageProps { @@ -35,10 +42,10 @@ export interface MessageProps { export function Message({kind = 'info', title, children, icon, onDismiss, className}: MessageProps) { return
{icon &&
{typeof icon === 'string' ? : } @@ -61,7 +68,7 @@ export function Message({kind = 'info', title, children, icon, onDismiss, classN // -------------------------------------------------------------------- Labels export interface LabelProps { - color?: SemanticColorName | 'black' | 'white'; + color?: SemanticColorName | RoleName | 'black' | 'white'; icon?: string | React.ComponentType; /** * Draw it as a physical tag — pointed left edge and an eyelet — rather than a plain @@ -100,7 +107,7 @@ export interface ProgressProps { indeterminate?: boolean; /** Show the percentage inside the bar. */ showPercent?: boolean; - color?: SemanticColorName; + color?: SemanticColorName | RoleName; /** Replaces the percentage with arbitrary text (e.g. "2.1 GB / 5.3 GB"). */ label?: React.ReactNode; className?: string; @@ -137,6 +144,19 @@ export function Progress({ export type StatusKind = 'complete' | 'active' | 'pending' | 'failed'; +/* + * Status was the one component already doing this correctly: it had per-theme overrides + * in ui.css because night has no hues to distinguish four states with. Those are gone -- + * the roles carry the brightness ramp now, so this table is the whole of it and every + * theme is served by the same rule. + */ +const statusRoles: Record = { + complete: 'success', + active: 'info', + pending: 'neutral', + failed: 'danger', +}; + const statusIcons: Record = { complete: 'check', active: 'circle notch', @@ -157,7 +177,10 @@ export interface StatusProps { * the color. */ export function Status({kind, children, plain}: StatusProps) { - return + return {!plain && } {children} diff --git a/app/src/components/ui/toast.test.js b/app/src/components/ui/toast.test.js index 338995c0..63323a8a 100644 --- a/app/src/components/ui/toast.test.js +++ b/app/src/components/ui/toast.test.js @@ -106,16 +106,33 @@ describe('toast', () => { await waitFor(() => expect(visibleToasts()).toHaveLength(0)); }); - it('carries the colour its type maps to', async () => { + it('carries the ROLE its type maps to, never a hue', async () => { withToasts(); showToast({type: 'error', title: 'Download failed'}); await waitFor(() => expect(visibleToasts()).toHaveLength(1)); - // The whole custom property, not a substring: `toContain('red')` would also be - // satisfied by any other declaration that happens to carry those three letters. + /* + * The whole custom property, not a substring: `toContain('danger')` would also be + * satisfied by any other declaration that happens to carry those letters. + * + * A hue here is the bug, not a detail. `red` resolves per theme, but in night + * `--yellow` and `--amber` are the same value and `--orange` is `--text`, so hue + * names cannot keep four kinds of toast apart there. The role can. + */ expect(visibleToasts()[0].getAttribute('style')) - .toMatch(/--notification-color:\s*var\(--mantine-color-red-/); + .toMatch(/--notification-color:\s*var\(--mantine-color-danger-/); + }); + + it('accepts `danger` as well as `error`, so new code can use the token name', async () => { + // `error` is what 26 call sites already write; both must reach the same role. + withToasts(); + + showToast({type: 'danger', title: 'Download failed'}); + + await waitFor(() => expect(visibleToasts()).toHaveLength(1)); + expect(visibleToasts()[0].getAttribute('style')) + .toMatch(/--notification-color:\s*var\(--mantine-color-danger-/); }); it('gives each type a colour of its own', async () => { @@ -145,7 +162,7 @@ describe('toast', () => { await waitFor(() => expect(visibleToasts()).toHaveLength(1)); expect(visibleToasts()[0].getAttribute('style')) - .toMatch(/--notification-color:\s*var\(--mantine-color-blue-/); + .toMatch(/--notification-color:\s*var\(--mantine-color-info-/); }); it('survives being called with nothing at all', () => { diff --git a/app/src/components/ui/toast.ts b/app/src/components/ui/toast.ts index 961b8629..eb9f46eb 100644 --- a/app/src/components/ui/toast.ts +++ b/app/src/components/ui/toast.ts @@ -1,5 +1,6 @@ import React from 'react'; import {notifications} from '@mantine/notifications'; +import {RoleName} from '../../themes/mantine'; /* * Toasts. @@ -9,7 +10,8 @@ import {notifications} from '@mantine/notifications'; * ThemeProvider. */ -export type ToastType = 'info' | 'success' | 'warning' | 'error'; +/* `error` is the original spelling and stays; `danger` is the token's name for it. */ +export type ToastType = 'info' | 'success' | 'warning' | 'error' | 'danger'; export interface ToastOptions { type?: ToastType; @@ -25,11 +27,18 @@ export interface ToastOptions { onClick?: () => void; } -const toastColors: Record = { - info: 'blue', - success: 'green', - warning: 'yellow', - error: 'red', +/* + * Roles, not hues. With hues, all four kinds were the same pixel in night and amber: + * `--yellow` there is byte-identical to `--amber`, and `--orange` to `--text`. An + * error toast was indistinguishable from an informational one, which is the failure + * mode a toast exists to avoid. + */ +const toastRoles: Record = { + info: 'info', + success: 'success', + warning: 'warning', + error: 'danger', + danger: 'danger', }; /** @@ -64,7 +73,7 @@ export function toast({type = 'info', title, description, time = 5000, onClick}: notifications.show({ title, message: description, - color: toastColors[type], + color: toastRoles[type], // Semantic treated 0 as "stay up"; Mantine wants false for that. Its `getAutoClose` // returns any number as given and the container then calls `setTimeout(hide, 0)`, so // passing the 0 through dismisses the toast on the next tick. diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index b72113fc..b633860c 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -1,12 +1,12 @@ import React from 'react'; import { ActionInput, Button, Header, Icon, IconStack, Loading, MultiSelect, Panel, PathInput, - Placeholder, Statistic, StatisticGroup, TabBar, Table, tabClassName, TextInput, + Placeholder, Statistic, StatisticGroup, Status, TabBar, Table, tabClassName, TextInput, } from './index'; import {contrastingColor} from '../Common'; import {Notifications} from '@mantine/notifications'; import {toast} from './toast'; -import {themeNames} from '../../themes/names'; +import {monochromeThemes, themeNames} from '../../themes/names'; /* `--panel` is authored as a hex; computed backgrounds come back as rgb(). */ const hexToRgb = (hex) => { @@ -175,6 +175,147 @@ const sampleTable =
; +describe('semantic roles read as severity in every theme', () => { + /* + * The point of the roles, and the only thing that makes them worth having. + * + * Light and dark spend a hue per role, so this is nearly free there. Night is red and + * nothing else and amber is amber and nothing else, so a role there is a step on a + * brightness ramp -- and it has to be, because `--orange` in night is byte-identical to + * `--text` and `--yellow` to `--amber`. Picking between hue names in night is picking + * between synonyms, which is why all four toast kinds were the same pixel. + * + * None of this is checkable in jsdom: every value is a token behind two levels of + * var(), and the claim is about resolved luminance. + */ + const ROLES = ['neutral', 'info', 'success', 'warning', 'danger']; + + const roleColours = () => { + const root = getComputedStyle(document.documentElement); + const panel = hexToRgb(root.getPropertyValue('--panel').trim()); + return { + panel, + text: hexToRgb(root.getPropertyValue('--text').trim()), + roles: ROLES.map(role => ({ + role, colour: hexToRgb(root.getPropertyValue(`--${role}`).trim()), + })), + }; + }; + + themeNames.forEach((theme) => { + it(`gives every role a colour of its own in ${theme}`, () => { + // True everywhere. Two roles resolving to the same value is the original bug. + cy.mountUI(roles, {theme}); + + cy.get('.wrolpi-panel').should(() => { + const {roles} = roleColours(); + const colours = roles.map(r => r.colour); + colours.forEach((colour, index) => { + expect(colour, `${roles[index].role} resolved`).to.match(/^rgb\(/); + }); + expect(new Set(colours).size, `five distinct roles, got ${colours.join(' ')}`) + .to.equal(5); + }); + }); + + it(`keeps every role legible in ${theme}`, () => { + cy.mountUI(roles, {theme}); + + cy.get('.wrolpi-panel').should(() => { + const {panel, roles} = roleColours(); + roles.forEach(({role, colour}) => { + /* + * 3:1 is the floor for anything that is not body text. `neutral` is + * allowed to sit at the bottom of the ramp -- for `pending` and disabled, + * being dim IS the signal -- but it still has to be visible at all. + */ + expect(contrast(colour, panel), `${role} against the panel`) + .to.be.at.least(role === 'neutral' ? 2.5 : 3); + }); + }); + }); + + + it(`gives each Status kind a colour of its own in ${theme}`, () => { + /* + * The end of the chain, through a real component: four states, four distinct + * painted colours. In night these used to need three hand-written + * `html[data-theme="night"]` overrides; the roles replaced all of them. + */ + cy.mountUI( + + complete + downloading + pending + failed + , + {theme}, + ); + + cy.get('.wrolpi-status').should(($all) => { + expect($all.length).to.equal(4); + const colours = [...$all].map(el => getComputedStyle(el).color); + expect(new Set(colours).size, `four distinct colours, got ${colours.join(' ')}`) + .to.equal(4); + }); + }); + }); + + monochromeThemes.forEach((theme) => { + it(`orders the roles by brightness in ${theme}, which is all it has`, () => { + /* + * Only here. Light and dark spend a hue per role, so their contrast order is + * an accident of which hues were chosen -- in light, blue is DARKER than green + * on white, so asserting a ramp there fails on a design that is perfectly fine. + * With hue gone, though, ordering is the only thing severity can be encoded in. + */ + cy.mountUI(roles, {theme}); + + cy.get('.wrolpi-panel').should(() => { + const {panel, roles} = roleColours(); + const ratios = roles.map(({role, colour}) => ({role, ratio: contrast(colour, panel)})); + ratios.slice(1).forEach((entry, index) => { + expect(entry.ratio, `${entry.role} is louder than ${ratios[index].role}`) + .to.be.greaterThan(ratios[index].ratio); + }); + }); + }); + + it(`makes danger at least as loud as ordinary text in ${theme}`, () => { + /* + * Also only here. A light theme's text is near-black at ~14:1 and no danger + * colour will ever match it; on a single hue the roles and the text are drawn + * from the same ramp, so the loudest role must not be quieter than the prose. + */ + cy.mountUI(roles, {theme}); + + cy.get('.wrolpi-panel').should(() => { + const {panel, text, roles} = roleColours(); + const danger = roles.find(r => r.role === 'danger').colour; + expect(contrast(danger, panel), 'danger vs --text against the panel') + .to.be.at.least(contrast(text, panel) * 0.95); + }); + }); + }); + + it('does not leave failure to colour alone', () => { + // Brightness is the first thing lost on a dim screen, and it is all night has. + cy.mountUI( + + failed + complete + , + {theme: 'night'}, + ); + + cy.get('.wrolpi-status-failed').should(($failed) => { + const other = Cypress.$('.wrolpi-status-complete')[0]; + expect(parseInt(getComputedStyle($failed[0]).fontWeight, 10), 'failed is heavier') + .to.be.greaterThan(parseInt(getComputedStyle(other).fontWeight, 10)); + }); + }); +}); + describe('a table header is a surface, not just bold text', () => { themeNames.forEach((theme) => { it(`separates the header from every body row in ${theme}`, () => { diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index b3daef1e..db428889 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -1139,46 +1139,26 @@ html[data-theme="amber"] .wrolpi-card-tag { /* ---------------------------------------------------------------- Status */ /* - * Status is encoded by hue in light/dark. In night mode hue is unavailable, so - * brightness carries it instead: dim = done, mid = active, brightest + bold = - * failed. Callers use these classes rather than naming a color. + * One rule for four states in four themes. The colour is the role the component + * chose, and the role already knows what it looks like here -- which is what + * replaced the three `html[data-theme="night"]` overrides that used to live below. */ - .wrolpi-status { font-size: 12.5px; font-weight: 500; display: inline-flex; align-items: center; gap: 5px; + color: var(--status-color); } -.wrolpi-status-complete { - color: var(--green); -} - -.wrolpi-status-active { - color: var(--blue); -} - -.wrolpi-status-pending { - color: var(--muted); -} - +/* + * Failure is not left to colour alone. Night and amber distinguish the roles by + * brightness, and brightness is the first thing lost on a dim screen or to a user + * who cannot separate these hues, so the loudest state also carries weight. + */ .wrolpi-status-failed { - color: var(--red); -} - -html[data-theme="night"] .wrolpi-status-complete { - color: var(--muted); -} - -html[data-theme="night"] .wrolpi-status-active { - color: var(--text); -} - -html[data-theme="night"] .wrolpi-status-failed { - color: var(--red); - font-weight: 500; + font-weight: 600; } /* ----------------------------------------------------------------- Tables */ diff --git a/app/src/themes/mantine.ts b/app/src/themes/mantine.ts index e1a29ee5..7e11bf21 100644 --- a/app/src/themes/mantine.ts +++ b/app/src/themes/mantine.ts @@ -19,6 +19,16 @@ import {createTheme, CSSVariablesResolver, MantineColorsTuple} from '@mantine/co const tokenColor = (token: string): MantineColorsTuple => Array(10).fill(`var(--${token})`) as unknown as MantineColorsTuple; +/** + * The semantic roles. A component asks for what a colour MEANS; the theme decides + * how that looks. Registered as Mantine colors too, so `color='danger'` works on any + * Mantine component the same way `color='red'` does. See the note in tokens.css -- + * in night and amber these are brightnesses, not hues. + */ +export const roleNames = ['neutral', 'info', 'success', 'warning', 'danger'] as const; + +export type RoleName = typeof roleNames[number]; + /** The Semantic UI color names WROLPi uses. Each maps to a token of the same name. */ export const semanticColorNames = [ 'red', 'orange', 'yellow', 'olive', 'green', 'teal', 'blue', 'violet', 'purple', 'pink', @@ -28,7 +38,7 @@ export const semanticColorNames = [ export type SemanticColorName = typeof semanticColorNames[number]; const semanticColors = Object.fromEntries( - semanticColorNames.map(name => [name, tokenColor(name)]) + [...semanticColorNames, ...roleNames].map(name => [name, tokenColor(name)]) ); export const mantineTheme = createTheme({ diff --git a/app/src/themes/names.ts b/app/src/themes/names.ts index 7d780894..7c06e157 100644 --- a/app/src/themes/names.ts +++ b/app/src/themes/names.ts @@ -24,6 +24,22 @@ export const darkThemes: ThemeName[] = [darkTheme, nightTheme, amberTheme]; /** Themes a user must choose deliberately; `prefers-color-scheme` never picks them. */ export const explicitOnlyThemes: ThemeName[] = [nightTheme, amberTheme]; +/* + * Themes built on a single hue, which is the constraint the semantic roles exist for. + * + * These have no second colour to spend, so a role cannot be a hue there -- it has to be + * a step on a brightness ramp, and `--orange` being byte-identical to `--text` in night + * is what that costs when it is ignored. Light and dark have hues and are free to use + * them, so severity does NOT read as brightness there and must not be asserted to. + * + * The same set as `explicitOnlyThemes` today, deliberately named separately: one is about + * how a theme is chosen, the other about what a theme has to work with. + */ +export const monochromeThemes: ThemeName[] = [nightTheme, amberTheme]; + +export const isMonochromeTheme = (theme: ThemeName): boolean => + monochromeThemes.includes(theme); + export const isDarkTheme = (theme: ThemeName): boolean => darkThemes.includes(theme); export const isThemeName = (value: unknown): value is ThemeName => diff --git a/app/src/themes/tokens.css b/app/src/themes/tokens.css index 0e62035d..0e2fc51d 100644 --- a/app/src/themes/tokens.css +++ b/app/src/themes/tokens.css @@ -50,19 +50,47 @@ html[data-theme="light"] { /* Switch thumbs and slider handles. */ --knob: #ffffff; -} -/* - * Role aliases. Declared once: `var()` resolves on , where each theme - * has already redefined the color it points at. - */ -:root { - --primary: var(--blue); + /* Roles: light has hues to spend, so each role is simply the hue that means it. */ + --neutral: var(--grey); + --info: var(--blue); --success: var(--green); --warning: var(--amber); --danger: var(--red); } +/* + * Semantic roles. + * + * What a colour MEANS, separate from what hue it happens to be. A call site asks + * for `--danger`, not `--red`, and each theme decides how danger looks. + * + * This matters because two of the four themes have no hues to spend. Night is red + * and nothing else; amber is amber and nothing else. There, a role is a step on a + * BRIGHTNESS ramp -- and it has to be, because `--orange` in night is byte-identical + * to `--text` and `--yellow` is byte-identical to `--amber`, so a call site choosing + * between hue names there is choosing between synonyms. That is why all four toast + * kinds looked the same in night, and why a warning and an error were the same pixel. + * + * The ramps below are built so that in every theme: + * - the order neutral < info < success < warning < danger is strictly increasing in + * contrast against the panel, so severity reads as brightness even without hue; + * - every role except neutral clears 3:1 against the panel; + * - danger reaches that theme's own `--text` brightness, so the loudest role is + * never quieter than ordinary prose; + * - neutral sits deliberately below the rest -- for `pending` and disabled, being + * dim IS the signal. + * + * `ui-layout.cy.js` asserts all of that, per theme, in a real browser. + * + * `--primary` is deliberately NOT one of these. "The colour of a primary button" and + * "the colour that means informational" are different jobs, and a user who recolours + * their buttons should not thereby recolour their messages. + */ +:root { + --primary: var(--blue); +} + html[data-theme="dark"] { --bg: #1c1f21; --panel: #26292b; @@ -91,6 +119,13 @@ html[data-theme="dark"] { --btn-text: #1c1f21; --knob: var(--text); + + /* Roles: as light -- a hue each. See the note above the :root block. */ + --neutral: var(--grey); + --info: var(--blue); + --success: var(--green); + --warning: var(--amber); + --danger: var(--red); } /* @@ -126,6 +161,13 @@ html[data-theme="night"] { --btn-text: #0a0000; --knob: var(--text); + + /* Roles: one hue, five brightnesses; saturation climbs with severity too. */ + --neutral: #992c2c; + --info: #c02929; + --success: #e02323; + --warning: #ee3434; + --danger: #ff3e3e; } /* Amber: a monochrome amber terminal palette, same flat style as the rest. */ @@ -160,6 +202,13 @@ html[data-theme="amber"] { /* Amber assigns the mono face to the body role; that is its character. */ --font-body: var(--font-system-mono); + + /* Roles: scaled to amber's own headroom (--text is 8.6:1, night's is 5.0:1). */ + --neutral: #815825; + --info: #b06e1f; + --success: #d37c11; + --warning: #f18806; + --danger: #ff9e29; } /* ------------------------------------------------------------------------- */