CORE-2008: Migrate feature components (MessageBox, Banner, Tree, ToggleButtonGroup)

CHANGELOG.md

@@ -30,6 +30,55 @@ ability to override the wrapper's CSS variables.
 
 ### Changed - BREAKING CHANGES
 
+#### Feature Component Migration (CORE-2008)
+
+`MessageBox`, `Banner`, `Tree` and `ToggleButtonGroup` have been migrated from styled-components
+to plain CSS bound to the `--ox-*` theme tokens. Props, behaviour and visual appearance are
+unchanged, but the exported pieces are no longer styled-components:
+
+**Breaking Changes:**
+
+1. **`BoxWrapper` (and `BoxHeading`, `BoxBody`, `BoxEventId`) are no longer styled-components**
+   - **Old behavior**: styled-components, usable as component selectors inside another styled
+     component's template — `${BoxWrapper} { ... }`
+   - **New behavior**: plain function components rendering `.message-box`, `.message-box-heading`,
+     `.message-box-body` and `.message-box-event-id`
+   - **Impact**: `${BoxWrapper}` in a styled-components template no longer resolves to a selector.
+     `openstax/assessments` does this in
+     `packages/frontend/src/assessments/screens/Preview/styled.tsx`
+   - **Migration**: target the `.message-box` class instead
+
+2. **`CloseButton` no longer wraps `Button`**
+   - **Old behavior**: `styled(Button)` that unset every style `Button` applied, and forwarded the
+     `severity` prop through to the rendered `<button>` element
+   - **New behavior**: a self-contained `<button class="banner-close-button">`. The old layering
+     only worked because styled-components injects its sheet last; in plain CSS the two class
+     selectors have equal specificity and the winner would depend on module evaluation order
+   - **Impact**: `variant` and `isWaiting` are no longer accepted (no consumer passes them), and the
+     rendered element no longer carries a `severity` attribute or the `button-base` class
+   - **Migration**: none needed for the `severity` / `onClick` / `aria-label` usage in the wild
+
+**Non-Breaking Changes:**
+
+- `StyledBanner`, `Severity`, `Tree`, `TreeItem`, `TreeItemContent`, `TreeChevron`, `MessageBox`
+  and `ToggleButtonGroup` keep their props and rendered structure.
+- Banner severity is now a tone class — `.banner-note`, `.banner-warning`, `.banner-error` — which
+  sets `--banner-bg`, `--banner-color` and `--banner-border-color`. The tone class is applied to
+  the close button as well as the banner, so `CloseButton` keeps its colour when rendered outside
+  a `StyledBanner`.
+- The react-aria wrappers (`Tree`, `TreeItem`, `StyledToggleButtonGroup`, `StyledToggleButton`)
+  compose `className` with `composeRenderProps`, so both the string and render-callback forms
+  survive. `styled(UI.ToggleButtonGroup)` in consuming projects keeps working.
+- `TreeItemContent` is re-exported straight from react-aria-components. It renders no DOM node, so
+  the empty `styled()` wrapper it used to carry was a no-op.
+- The `style` prop on the `MessageBox` and `Banner` exports is widened from `React.CSSProperties`
+  to `CSSPropertiesWithVariables`, so callers can set the documented `--message-box-*` and
+  `--banner-*` variables without casting. This is a widening, so existing usage is unaffected.
+- Four Banner colours (`#fff5e0`, `#976502`, `#fdbd3e`, `#f8e8ea`) are recorded in
+  `KNOWN_OFF_PALETTE` rather than snapped to the nearest palette entry, which would have been a
+  visual change.
+
+
 #### Button Component Migration (CORE-1999)
 
 The Button component and its variants have been migrated from styled-components to standard CSS with CSS custom properties. While the components maintain the same visual appearance and React API, there are **breaking changes** for certain exports:

README.md

@@ -51,6 +51,96 @@ npm run lint       # Check code rules
 npm run dist       # Build distribution files
 ```
 
+## Styling
+
+Components are styled with plain CSS in a sibling `.css` file, imported for side effects.
+
+### Theme tokens
+
+Theme values are defined once, in JavaScript, and projected into CSS custom properties by
+`src/theme/theme.css`. That file is **generated** from `src/theme/palette.ts` and
+`src/theme.ts` — never edit it by hand. `npm run build` regenerates it as its first step, so
+a published package can never ship a stale one. To refresh it without a full build:
+
+```
+npm run generate:theme-css
+```
+
+It is committed as well as generated, because jest and ladle read `src/` directly and never
+run the build — `src/theme/tokens.spec.ts` fails when the committed copy is stale, which is
+what makes CI (which runs lint and test, not build) catch it.
+
+**Never write a theme value as a literal in a component stylesheet** — reference the token
+instead:
+
+```css
+/* no */
+.thing { border-color: #d5d5d5; }
+
+/* yes */
+.thing { border-color: var(--ox-color-pale); }
+```
+
+Tokens are `--ox-`-prefixed, so they will not collide with a consuming app's own variables.
+Colour tokens are the kebab-case form of the `src/theme/palette.ts` key
+(`palette.neutralLighter` → `--ox-color-neutral-lighter`); there are also `--ox-color-link`,
+`--ox-color-link-hover`, `--ox-z-index-*` and `--ox-padding-navbar-*`.
+
+Any component whose CSS uses a token must import the token file alongside its own stylesheet:
+
+```ts
+import './MyComponent.css';
+import '../theme/theme.css';
+```
+
+Consumers need do nothing — bundlers deduplicate the import, and `sideEffects` in
+`package.json` keeps it from being tree-shaken.
+
+### Component override hooks and when to bind in JS
+
+A component may expose its own `--component-*` custom property so consumers can restyle it.
+Declare the **default in the stylesheet** with the token as the fallback, and do not bind it
+from JavaScript:
+
+```css
+.tabs [role="tab"] { border-color: var(--tabs-active-border-color, var(--ox-color-dark-green)); }
+```
+
+Bind a custom property from JavaScript only when its value genuinely varies at runtime — a
+variant lookup, a numeric prop, a disabled state. A static colour pushed through an inline
+style is duplication with extra steps, and it wins over the cascade in ways callers do not
+expect.
+
+Widen the component's `style` prop to `CSSPropertiesWithVariables` (from `src/types`) so
+callers can set these without a cast.
+
+### What CI enforces
+
+`src/theme/tokens.spec.ts` fails the build if:
+
+- the committed `theme.css` is not what the generator produces from the JS theme (the build
+  regenerates it; this is what stops a stale copy reaching jest, ladle or a reviewer)
+- a component stylesheet writes a colour literal that duplicates a theme value
+- a component stylesheet introduces a colour that is not a theme value and not in the
+  `KNOWN_OFF_PALETTE` allowlist
+- a component stylesheet reads an `--ox-*` token that does not exist
+
+The colour check parses declarations, so it covers every syntax a colour can be written in —
+hex, `rgb()`/`hsl()`/`oklch()`/`color()`, and bare named colours wherever they appear,
+including inside shorthands and gradient stops. Functions that merely *contain* colours
+(`var()`, `color-mix()`, the gradients) are descended into rather than treated as literals,
+so building a value out of tokens stays clean. A translucent colour is accepted when its
+opaque channels are a theme value — `rgba(0, 0, 0, 0.2)` is black at 20% and there is no
+token form for it — which still refuses a new hue smuggled in through `rgba()`.
+
+The check has its own tests, so the guarantee is a tested one rather than an asserted one.
+
+Adding a genuinely new colour is therefore a deliberate act: put it in `palette.ts` if it is
+part of the design, or in the allowlist with a reason if it is a one-off we are keeping.
+
+Breakpoints are the known gap — `@media (min-width: var(--x))` is not valid CSS, so
+breakpoint values are still repeated in media queries and are not covered by the tests.
+
 ## Testing Across Projects
 
 To test changes before tagging:

package.json

@@ -40,6 +40,7 @@
   ],
   "scripts": {
     "build": "./scripts/build.bash",
+    "generate:theme-css": "./scripts/generate-theme-css.bash",
     "clean": "rm -rf ./dist",
     "build:clean": "npm-run-all clean build",
     "dist": "npm-run-all clean build",

scripts/build.bash

@@ -6,6 +6,11 @@ project_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." >/dev/null 2>&1 && pwd
 
 cd "$project_dir"
 
+# theme.css is generated from the JS theme, so regenerate before anything copies it.
+# Committed too, because jest and ladle read src/ directly and never run this script;
+# src/theme/tokens.spec.ts fails when the committed file is stale.
+./scripts/generate-theme-css.bash
+
 tsc_args=(--noEmit false --declaration)
 
 npx tsc --project tsconfig.without-specs.esm.json "${tsc_args[@]}"

scripts/generate-theme-css.bash

@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+# spell-checker: ignore pipefail outdir
+#
+# Regenerates src/theme/theme.css from the JavaScript theme. The committed file is checked
+# against this output by src/theme/tokens.spec.ts, so CI fails if palette.ts or theme.ts
+# changes without this being re-run.
+set -euo pipefail; if [ -n "${DEBUG-}" ]; then set -x; fi
+
+project_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." >/dev/null 2>&1 && pwd )"
+cd "$project_dir"
+
+# The theme is TypeScript and the repo has no TS runner outside jest, so compile the
+# renderer and its (small, pure) import graph to a throwaway directory and run that.
+outdir="$(mktemp -d)"
+trap 'rm -rf "$outdir"' EXIT
+
+npx tsc src/theme/themeCss.ts \
+  --outDir "$outdir" --rootDir . \
+  --module commonjs --target es2019 --moduleResolution node --esModuleInterop --skipLibCheck
+
+node -e "
+  const fs = require('fs');
+  const { renderThemeCss } = require('$outdir/src/theme/themeCss');
+  fs.writeFileSync('src/theme/theme.css', renderThemeCss());
+  console.log('wrote src/theme/theme.css');
+"

src/components/Banner/Banner.css

@@ -0,0 +1,70 @@
+/*
+ * Banner styles.
+ *
+ * The severity tone lives in the `banner-note` / `banner-warning` / `banner-error`
+ * classes, which set the three --banner-* custom properties. Both the banner and its
+ * close button carry the tone class, so the button keeps its severity colour even when a
+ * consumer renders it outside a StyledBanner.
+ */
+.banner-note,
+.banner-warning {
+  --banner-bg: #fff5e0;
+  --banner-color: #976502;
+  --banner-border-color: #fdbd3e;
+}
+
+.banner-error {
+  --banner-bg: #F8E8EA;
+  --banner-color: var(--ox-color-dark-red);
+  --banner-border-color: var(--ox-color-light-red);
+}
+
+.banner {
+  position: relative;
+  background: var(--banner-bg);
+  color: var(--banner-color);
+  border: 1px solid var(--banner-border-color);
+  padding: .6rem 1.6rem;
+  margin: 0 0 1.6rem 0;
+  line-height: 2rem;
+  display: flex;
+  align-items: center;
+}
+
+.banner a {
+  text-decoration: none;
+  color: var(--banner-link-color, var(--ox-color-medium-blue));
+}
+
+.banner a:hover {
+  text-decoration: underline;
+  color: var(--banner-link-hover-color, var(--ox-color-link-hover));
+}
+
+.banner .button-link {
+  font-size: 1.6rem;
+}
+
+.banner-severity {
+  font-weight: bold;
+  text-transform: uppercase;
+}
+
+/*
+ * Self-contained rather than layered over .button-base: the styled-components original
+ * wrapped Button and then unset everything Button gave it, which only worked because
+ * styled-components injected its sheet last. Plain CSS has no such guarantee, and the two
+ * class selectors would have equal specificity.
+ */
+.banner-close-button {
+  color: var(--banner-color);
+  overflow: visible;
+  background: none;
+  border: none;
+  padding: 0;
+  font: inherit;
+  cursor: pointer;
+  outline: inherit;
+  box-shadow: none;
+  margin-left: 2.4rem;
+}

src/components/Banner/Banner.spec.tsx

@@ -1,4 +1,5 @@
-import { Banner } from "./Banner";
+import { render } from '@testing-library/react';
+import { Banner, CloseButton, StyledBanner } from "./Banner";
 import renderer from 'react-test-renderer';
 
 describe('Banner', () => {
@@ -30,4 +31,37 @@ describe('Banner', () => {
     ).toJSON();
     expect(tree).toMatchSnapshot();
   });
-});
+
+  it.each([
+    ['note', 'banner-note'],
+    ['warning', 'banner-warning'],
+    ['error', 'banner-error'],
+  ] as const)('puts the %s tone class on the banner', (severity, expected) => {
+    render(<Banner messages={['a message']} severity={severity} />);
+
+    expect(document.querySelector('.banner')?.className).toContain(expected);
+  });
+
+  it('puts the tone class on the close button too, so it works outside a banner', () => {
+    render(<CloseButton severity='error' aria-label='dismiss' />);
+
+    const button = document.querySelector('.banner-close-button');
+    expect(button?.className).toContain('banner-error');
+  });
+
+  it('does not leak the severity prop to the DOM', () => {
+    // styled(Button) forwarded unknown props through to the underlying element, so the
+    // rendered button used to carry severity="warning".
+    render(<Banner messages={['a message']} severity='warning' onDismiss={() => null} />);
+
+    expect(document.querySelector('.banner-close-button')?.hasAttribute('severity')).toBe(false);
+  });
+
+  it('composes a caller className rather than replacing it', () => {
+    render(<StyledBanner severity='note' className='caller-banner' />);
+
+    const banner = document.querySelector('.banner');
+    expect(banner?.className).toContain('banner-note');
+    expect(banner?.className).toContain('caller-banner');
+  });
+});

src/components/Banner/Banner.stories.tsx

@@ -1,13 +1,16 @@
 import React from 'react';
 import { Banner } from './Banner';
-import styled from 'styled-components';
 
-const BannerContainer = styled.div`
-  font-size: 1.2rem;
-  position: relative;
-  padding-right: 2.5rem;
-  width: 42rem;
-`;
+const containerStyle: React.CSSProperties = {
+  fontSize: '1.2rem',
+  position: 'relative',
+  paddingRight: '2.5rem',
+  width: '42rem',
+};
+
+const BannerContainer = ({ children }: React.PropsWithChildren<unknown>) => (
+  <div style={containerStyle}>{children}</div>
+);
 
 export const Error = () => (
   <BannerContainer>
@@ -44,4 +47,4 @@ export const Dismissible = () => {
       />
     </BannerContainer>
   ) : null;
-};
+};