Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 94 additions & 48 deletions PLAIN_CSS_MIGRATION_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -212,39 +212,41 @@ export function Button({ children, ...props }) {
}
```

### Pattern 2.5: Using Root-Level CSS Variables for Static Theme Colors
### Pattern 2.5: Using Global Theme Tokens for Static Theme Values

For static theme colors that don't require dynamic property access (like `theme.color.text.*`, `theme.color.neutral.*`, `theme.color.disabled.*`), use root-level CSS variables defined in `src/index.css` instead of binding them at the component level.
For theme values that don't require dynamic property access, reference the global
tokens in `src/app/theme.css` instead of binding them at the component level.

**Benefits:**
- Reduces code duplication across components
- Improves maintainability by centralizing static color definitions
- Establishes clear patterns for future migrations
- Keeps a documented mapping from `src/app/theme.ts` into shared CSS variables
**`src/app/theme.css` is generated — do not edit it.** `src/app/themeData.ts` is the
source of truth, `src/app/themeCss.ts` owns the projection, and
`yarn generate:theme-css` writes the file. It is also regenerated as part of
`yarn build` and `yarn start`, so a build cannot ship a stale copy.

**Important:** The root-level variables in `src/index.css` are copied from `src/app/theme.ts`; they are not automatically generated or verified by the process described in this guide. When updating these values, keep `src/index.css` and `src/app/theme.ts` in sync manually.
**When to use root-level variables:**
- Static colors that never change based on props
- Colors used across multiple components
- Colors from `theme.color.text.*`, `theme.color.neutral.*`, `theme.color.disabled.*`, and `theme.color.primary.gray.*`
The token families are `--color-*`, `--z-index-*` and `--padding-*`. Anything else
unprefixed (`--section-bg`, `--popup-padding`) is a component-local override hook, not
a global token.

**When to use component-level bindings:**
**When to use a global token:**
- Any static color, z-index or page padding — i.e. the value does not depend on props
- Values used across more than one component

**When to bind from JavaScript instead:**
- Book-specific theme colors requiring dynamic property access (`theme.color.primary[bookTheme]`)
- Colors with runtime computations (highlight colors using Color library)
- Any color that changes based on props or state
- Colors with runtime computations (highlight colors via the Color library)
- Any value that changes based on props or state

**Example:**

```typescript
// ❌ Before: Component-level binding for static color
// ❌ Before: component-level binding for a static color
import theme from '../theme';

export function Card({ className, style, ...props }) {
return (
<div
className="modal-card"
style={{
'--text-color': theme.color.text.default, // Static, repeated across components
'--text-color': theme.color.text.default, // static, repeated across components
...style,
} as React.CSSProperties}
/>
Expand All @@ -253,43 +255,70 @@ export function Card({ className, style, ...props }) {
```

```typescript
// ✅ After: Use root-level CSS variable
// ✅ After: reference the token in CSS
export function Card({ className, style, ...props }) {
return (
<div
className="modal-card"
style={style} // No need to bind static colors
/>
);
return <div className="modal-card" style={style} />;
}
```

```css
/* Component.css */
.modal-card {
color: var(--color-text-default); /* References root-level variable */
color: var(--color-text-default);
background: var(--color-neutral-base);
}
```

**Naming Convention:**
Note that removing an inline default changes cascade precedence: a rule setting the
variable on an ancestor element used to lose to the inline style and now applies. Grep
for the variable before removing its binding.

**Naming convention:**

A token name is the kebab-case form of its path in `themeData.ts`:

- `color.text.default` → `--color-text-default`
- `color.neutral.pageBackground` → `--color-neutral-page-background`
- `color.primary.gray.base` → `--color-primary-gray-base`
- `zIndex.navbar` → `--z-index-navbar`
- `padding.page.desktop` → `--padding-page-desktop` (emitted in `rem`)

The one exception is `--color-link`, rather than `--color-link-base`.

See `src/app/theme.css` for the full list of 80 tokens.

**What CI enforces** (`src/app/theme.spec.ts`):

Root-level CSS variables follow the pattern: `--color-{category}-{property}`
1. The committed `theme.css` matches the generator's output, so the CSS cannot go stale
against the JS. If this fails, run `yarn generate:theme-css`.
2. No stylesheet writes a color literal that duplicates a theme value.
3. No stylesheet introduces a color that is neither a theme value nor explicitly
allowlisted in `KNOWN_OFF_PALETTE` (in `src/test/cssColors.ts`) with a reason.
4. No stylesheet reads a `--color-*`/`--z-index-*`/`--padding-*` token that doesn't
exist — which also keeps component-local variables out of those namespaces.
5. No `@media` breakpoint sits within 1em of a theme breakpoint without being one,
which catches a `75em` mistyped as `74em`. The bound is inclusive, so `74em` and
`76em` are both caught.

- `theme.color.text.default` → `--color-text-default`
- `theme.color.neutral.pageBackground` → `--color-neutral-page-background`
- `theme.color.primary.gray.base` → `--color-primary-gray-base`
- `theme.color.disabled.foreground` → `--color-disabled-foreground`
Checks 2 and 3 are currently locked to a baseline in `src/app/theme.baseline.json`,
recording the violations that predate the token file. **New** violations fail
immediately; the baseline only shrinks. If you remove some, run
`yarn generate:theme-baseline` — the counts it prints should go down.

Note: camelCase properties in theme.ts become kebab-case in CSS variable names.
Each baseline entry names the file, the selector and at-rule it sits under, and the
property, not just the literal — `app/components/Button.css: .btn:hover { color: #fff }
is --color-neutral-base`. That is what stops a removed `#fff` and a newly added one
elsewhere in the same file from cancelling out. Expect the entry to change, and the
baseline to need regenerating, if you move a declaration or rename a selector; the
counts are what should not go up.

**Available Root-Level Variables:**
Hex literals and `rgb()`/`hsl()`/`oklch()` are audited in every declaration. Bare named
colors (`white`, `tan`) are only read as colors in properties that can take one, so
`animation-name: red` is left alone. `src/test/cssColors.ts` has the list.

See `src/index.css` for the complete list of available root-level CSS variables. These include:
- Text colors: `--color-text-black`, `--color-text-default`, `--color-text-label`, `--color-text-white`
- Neutral colors: `--color-neutral-base`, `--color-neutral-darker`, `--color-neutral-darkest`, `--color-neutral-page-background`, etc.
- Disabled colors: `--color-disabled-base`, `--color-disabled-foreground`
- Primary gray colors: `--color-primary-gray-base`, `--color-primary-gray-darker`, etc.
**Breakpoints are the known gap.** `@media (min-width: var(--x))` is not valid CSS, so
breakpoint values stay duplicated in stylesheets (`75em` appears well over a hundred
times). Check 5 is the cheap mitigation; a real fix needs a preprocessor step.

### Pattern 3: Component with Dynamic Theme Access

Expand Down Expand Up @@ -548,19 +577,33 @@ test('renders button with correct class', () => {

## Common Pitfalls

### 1. Don't Duplicate Theme in CSS
### 1. Don't Duplicate Theme Values in CSS

**❌ Wrong:**
**❌ Wrong:** hand-copying the value. `src/app/theme.spec.ts` fails on this.
```css
.button {
background: #d4450c; /* duplicates color.primary.orange.base */
}
```

**❌ Also wrong:** declaring your own token for it. `theme.css` is generated; a second
declaration is a second source of truth.
```css
:root {
--color-orange: #d4450c; /* Duplicates theme.ts */
--color-orange: #d4450c;
}
```

**✅ Right:**
**✅ Right:** reference the generated token.
```css
.button {
background: var(--color-primary-orange-base);
}
```

**✅ Also right,** when the value genuinely depends on props — bind it from JavaScript:
```typescript
// Bind from theme.ts at component level
style={{ '--button-bg': theme.color.primary.orange.base }}
style={{ '--button-bg': theme.color.primary[bookTheme].base }}
```

### 2. Use classNames Library for Conditional Classes
Expand Down Expand Up @@ -633,11 +676,14 @@ const colors = theme.color.primary[bookTheme];
style={{ '--banner-bg': colors.base }}
```

### 6. Remember That theme.ts is Still the Source of Truth
### 6. Remember Where the Source of Truth Is

- Don't hardcode theme values in CSS
- Always reference theme.ts when you need theme values
- The theme object is unchanged and fully accessible in JavaScript
- `src/app/themeData.ts` holds the values. `src/app/theme.css` is **generated** from it
— never edit the CSS by hand, and run `yarn generate:theme-css` after changing the data.
- `theme.ts` re-exports that data, so `theme.color.x` in JavaScript is unchanged.
- Don't hardcode a theme value in CSS, and don't declare a second token for one.
- Link colors come from the theme too; import them from
`components/Typography/Links.constants.ts` in JS, or use `var(--color-link)` in CSS.

## Migration Checklist

Expand Down
12 changes: 6 additions & 6 deletions e2e_tests/e2e/ui/pages/home.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,28 +232,28 @@ async def fill_highlight_box_note_field(self, value):
await self.page.locator("#note-textarea").fill(value)

@pytest.mark.asyncio
async def highlight_box_colours_are_visible(self):
async def highlight_box_colors_are_visible(self):
return (
await self.page.locator("div")
.get_by_test_id("highlight-colours-picker")
.get_by_test_id("highlight-colors-picker")
.is_visible()
)

@pytest.mark.asyncio
async def click_highlight_box_purple_colour(self):
async def click_highlight_box_purple_color(self):
await self.page.locator("div").get_by_title("purple").first.click()

@pytest.mark.asyncio
async def click_highlights_option_green_colour(self):
async def click_highlights_option_green_color(self):
await self.page.locator("div").get_by_title("green").first.click()

@property
def highlights_option_text_colour_check_purple(self):
def highlights_option_text_color_check_purple(self):
return self.page.locator(".highlight-content-wrapper").filter(
has_text="notepurple")

@property
def highlights_option_text_colour_check_green(self):
def highlights_option_text_color_check_green(self):
return self.page.locator(".highlight-content-wrapper").filter(
has_text="notegreen")

Expand Down
12 changes: 6 additions & 6 deletions e2e_tests/e2e/ui/test_highlight_box_save_note.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ async def test_overlapping_highlights(
@pytest.mark.parametrize(
"book_slug, page_slug", [("astronomy-2e", "9-3-impact-craters")]
)
async def test_highlight_box_note_colours(
async def test_highlight_box_note_colors(
chrome_page, base_url, book_slug, page_slug, rex_user, rex_password
):

Expand All @@ -98,7 +98,7 @@ async def test_highlight_box_note_colours(
await chrome_page.goto(f"{base_url}/books/{book_slug}/pages/{page_slug}")
home = HomeRex(chrome_page)

# THEN: Book page opens, highlight box appears with colours and highlighted text can get different colour
# THEN: Book page opens, highlight box appears with colors and highlighted text can get different color
await chrome_page.keyboard.press("Escape")

await home.select_text()
Expand All @@ -108,7 +108,7 @@ async def test_highlight_box_note_colours(

assert await home.highlight_box_is_visible()

await home.click_highlight_box_purple_colour()
await home.click_highlight_box_purple_color()

await home.click_highlight_box_note_field()

Expand All @@ -125,15 +125,15 @@ async def test_highlight_box_note_colours(
in await home.highlights_option_page.inner_text()
)

assert await home.highlights_option_text_colour_check_purple.is_visible()
assert await home.highlights_option_text_color_check_purple.is_visible()

await home.click_highlights_option_page_menu()

await home.click_highlights_option_green_colour()
await home.click_highlights_option_green_color()

await chrome_page.keyboard.press("Escape")

assert await home.highlights_option_text_colour_check_green.is_visible()
assert await home.highlights_option_text_color_check_green.is_visible()

# THEN: Delete the created highlight

Expand Down
2 changes: 1 addition & 1 deletion e2e_tests/e2e/ui/test_highlight_editbox.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ async def test_highlight_editbox_opens_on_one_click(

assert await home.highlight_box_is_visible()

assert await home.highlight_box_colours_are_visible()
assert await home.highlight_box_colors_are_visible()
assert await home.highlight_box_trash_icon_is_visible()

await home.click_highlight_box_trash_icon()
Expand Down
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,14 @@
"lint:css:plain": "stylelint --config .stylelintrc.css.json 'src/**/*.css'",
"lint:css:plain-fix": "stylelint --fix --config .stylelintrc.css.json 'src/**/*.css'",
"lint:bash": "shellcheck $(find . -type f \\( -iname '*\\.sh' -or -iname '*\\.bash' \\) | grep -v -e .venv -e node_modules)",
"prestart": "npm run-script build:css",
"prestart": "npm run-script build:css && npm run-script generate:theme-css",
"start": "./script/start.bash",
"start:static": "export REACT_APP_ENV=${REACT_APP_ENV:-test} && npm run-script build && npm run-script prerender:local && npm run-script server",
"clean": "rm -rf ./build",
"build": "export DISABLE_NEW_JSX_TRANSFORM=true && npm run-script build:css && npm run-script build:js",
"build": "export DISABLE_NEW_JSX_TRANSFORM=true && npm run-script build:css && npm run-script generate:theme-css && npm run-script build:js",
"build:css": "lessc --source-map --source-map-include-source ./generic-styles/index.less ./src/content.css",
"generate:theme-css": "node ./script/entry generate-theme-css",
"generate:theme-baseline": "node ./script/entry generate-theme-baseline",
"build:js": "export REACT_APP_ENV=${REACT_APP_ENV:-production} && GENERATE_SOURCEMAP=${GENERATE_SOURCEMAP:-true} craco build",
"build:clean": "npm run-script clean && npm run-script build",
"prerender:local": "REACT_APP_ENV=${REACT_APP_ENV:-production} node ./script/entry prerender/local",
Expand Down
23 changes: 23 additions & 0 deletions script/generate-theme-baseline.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* Rewrites src/app/theme.baseline.json from the current stylesheets.
*
* The baseline records the color violations that predate the token file, so that
* src/app/theme.spec.ts can fail on *new* ones while the sweep works through the old
* ones. Run this after removing violations — the counts it prints should go down, never
* up. If they go up, you have added a hardcoded color that belongs in a token.
*/
import fs from 'fs';
import path from 'path';
import { colorViolations } from '../src/test/cssColors';

const srcDir = path.join(__dirname, '..', 'src');
const target = path.join(srcDir, 'app', 'theme.baseline.json');
const violations = colorViolations(srcDir);

fs.writeFileSync(target, `${JSON.stringify(violations, null, 2)}\n`);

// eslint-disable-next-line no-console
console.log(
`wrote ${path.relative(process.cwd(), target)}: `
+ `${violations.duplicates.length} duplicates, ${violations.unknown.length} unrecognised`
);
14 changes: 14 additions & 0 deletions script/generate-theme-css.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Writes src/app/theme.css from the JS theme. Run via `yarn generate:theme-css`.
* The projection itself lives in src/app/themeCss.ts so that it can be unit tested.
*/
import fs from 'fs';
import path from 'path';
import { themeCss } from '../src/app/themeCss';

const target = path.join(__dirname, '..', 'src', 'app', 'theme.css');

fs.writeFileSync(target, themeCss());

// eslint-disable-next-line no-console
console.log(`wrote ${path.relative(process.cwd(), target)}`);
21 changes: 13 additions & 8 deletions src/app/components/Typography/Links.constants.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
/**
* Link Color Constants
*
* Single source of truth for link colors used across Typography components.
* These constants are:
* - Imported by Button.tsx (ButtonLink component) and bound as CSS variables
* - Imported by Typography.legacy.ts for styled-components css fragments
* - Imported by NavBar/index.tsx for focus outline color
* Re-exported from `app/themeData`, which is the single source of truth and the
* origin of the `--color-link-*` CSS tokens. This module remains the import site
* for JS consumers:
* - Button.tsx (ButtonLink component) binds them as CSS variables
* - Typography.legacy.ts uses them in styled-components css fragments
* - NavBar/index.tsx uses linkFocusOutline for its focus outline color
*
* Stylesheets must not hand-copy these values; use var(--color-link),
* var(--color-link-hover) or var(--color-link-focus-outline) instead.
*
* This module has no side effects (no React, no CSS imports).
*/
import { linkColors } from '../../themeData';

export const linkColor = '#027EB5';
export const linkHover = '#0064A0';
export const linkFocusOutline = '#007297';
export const linkColor = linkColors.base;
export const linkHover = linkColors.hover;
export const linkFocusOutline = linkColors.focusOutline;
Loading
Loading