feat(ui): add theme stories and support#2541
Merged
Merged
Conversation
Contributor
Greptile SummaryThis PR refactors the UI package's surface token system from an arbitrary naming scheme (
Confidence Score: 4/5The changes are well-scoped to the UI package theme layer and Storybook; no runtime logic or data paths are affected. The token system, CSS variable chain, and component migrations are internally consistent and the Storybook stories provide a good visual regression harness. The button.tsx secondary variant was not migrated alongside outline/ghost, which may cause a visual inconsistency on surfaces where background-2 differs from the surface context token; the watch script has a minor misleading log message at startup. packages/ui/src/components/button.tsx — the secondary variant's bg-background-2 may need migrating; packages/ui/src/theme/watch.mjs — startup log message.
|
| Filename | Overview |
|---|---|
| packages/ui/src/theme/watch.mjs | New dev-only watcher that rebuilds the theme on tokens.json changes; the initial-build log message says token change detected which is misleading on startup. |
| packages/ui/src/components/button.tsx | Migrates outline and ghost variants to surface-aware tokens; secondary variant still uses old bg-background-2 token. |
| packages/ui/src/stories/surfaces.stories.tsx | New story file with Palette, ComponentsOnAllSurfaces, and BothModes stories to visually validate all surface families. |
| packages/ui/src/theme/surfaces.css | Renames surface scope classes to the new elevation vocabulary and adds a :root default binding to surface-base. |
| packages/ui/src/theme/tailwind.css | Switches --color-surface/hover/selected to indirect CSS variable references (context-adaptive), and adds direct elevation aliases for all five levels. |
| packages/ui/src/theme/theme.css | Defines new semantic elevation token values for both light and dark modes across all three color-mode blocks. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
TJ["tokens.json"]
TC["theme.css"]
SC["surfaces.css"]
TW["tailwind.css"]
COMP["Components"]
WATCH["watch.mjs"]
TJ -->|"terrazzo build"| TC
TC --> SC
SC --> TW
TW --> COMP
WATCH -.->|"triggers rebuild on save"| TJ
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
TJ["tokens.json"]
TC["theme.css"]
SC["surfaces.css"]
TW["tailwind.css"]
COMP["Components"]
WATCH["watch.mjs"]
TJ -->|"terrazzo build"| TC
TC --> SC
SC --> TW
TW --> COMP
WATCH -.->|"triggers rebuild on save"| TJ
Comments Outside Diff (1)
-
packages/ui/src/components/button.tsx, line 37 (link)Secondary variant not migrated to surface tokens
The
secondaryvariant still referencesbg-background-2whileoutlineandghostwere both updated tobg-surface/bg-surface-hover. If this was intentional (e.g. secondary intentionally stays outside the surface context), it's worth a comment explaining why; otherwise it may be an overlooked migration.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/ui/src/components/button.tsx Line: 37 Comment: **Secondary variant not migrated to surface tokens** The `secondary` variant still references `bg-background-2` while `outline` and `ghost` were both updated to `bg-surface` / `bg-surface-hover`. If this was intentional (e.g. secondary intentionally stays outside the surface context), it's worth a comment explaining why; otherwise it may be an overlooked migration. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
packages/ui/src/theme/watch.mjs:25-26
The `runBuild()` function always logs "token change detected" even when called at startup before any file has changed. A developer running `theme:watch` for the first time will see a misleading message.
```suggestion
function runBuild(reason = 'token change detected') {
console.log(`\n${reason} — rebuilding theme…`);
```
### Issue 2 of 5
packages/ui/src/theme/watch.mjs:16-17
Calling `runBuild()` without an argument at startup will still print "token change detected" with the default. Pass a descriptive reason so the initial run is clearly distinguished from watch-triggered rebuilds.
```suggestion
// Run once on start so the generated files are always fresh.
runBuild('initial startup');
```
### Issue 3 of 5
packages/ui/src/theme/watch.mjs:1-9
The `execSync` import is placed before the JSDoc comment that describes the module, splitting the import block. Moving it with the other imports keeps the file header readable and consistent.
```suggestion
/**
* Watches tokens.json and re-runs theme:build whenever it changes.
* Run via: pnpm run theme:watch
* Storybook HMR then picks up the updated theme.css automatically.
*/
import { execSync } from 'node:child_process';
import { watch } from 'node:fs';
import { resolve, dirname } from 'node:path';
import { fileURLToPath } from 'node:url';
```
### Issue 4 of 5
packages/ui/src/components/button.tsx:37
**Secondary variant not migrated to surface tokens**
The `secondary` variant still references `bg-background-2` while `outline` and `ghost` were both updated to `bg-surface` / `bg-surface-hover`. If this was intentional (e.g. secondary intentionally stays outside the surface context), it's worth a comment explaining why; otherwise it may be an overlooked migration.
### Issue 5 of 5
packages/ui/src/stories/surfaces.stories.tsx:104-119
With 5 families and `grid-cols-2`, the last card renders as a lone item in its own row in both the light and dark panes. Using `grid-cols-3` would keep the layout balanced (3+2) while still fitting in a half-screen column.
```suggestion
<div className="emlight flex-1 space-y-6 bg-background p-8">
<p className="text-sm font-medium text-foreground">Light mode</p>
<div className="grid grid-cols-3 gap-3">
{FAMILIES.map((f) => (
<SurfaceCard key={f} family={f} />
))}
</div>
</div>
<div className="emdark flex-1 space-y-6 bg-background p-8">
<p className="text-sm font-medium text-foreground">Dark mode</p>
<div className="grid grid-cols-3 gap-3">
{FAMILIES.map((f) => (
<SurfaceCard key={f} family={f} />
))}
</div>
</div>
```
Reviews (1): Last reviewed commit: "feat: add theme stories and support" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+25
to
+26
| function runBuild() { | ||
| console.log('\ntoken change detected — rebuilding theme…'); |
Contributor
There was a problem hiding this comment.
runBuild() function always logs "token change detected" even when called at startup before any file has changed. A developer running theme:watch for the first time will see a misleading message.
Suggested change
| function runBuild() { | |
| console.log('\ntoken change detected — rebuilding theme…'); | |
| function runBuild(reason = 'token change detected') { | |
| console.log(`\n${reason} — rebuilding theme…`); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/theme/watch.mjs
Line: 25-26
Comment:
The `runBuild()` function always logs "token change detected" even when called at startup before any file has changed. A developer running `theme:watch` for the first time will see a misleading message.
```suggestion
function runBuild(reason = 'token change detected') {
console.log(`\n${reason} — rebuilding theme…`);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+16
to
+17
| // Run once on start so the generated files are always fresh. | ||
| runBuild(); |
Contributor
There was a problem hiding this comment.
runBuild() without an argument at startup will still print "token change detected" with the default. Pass a descriptive reason so the initial run is clearly distinguished from watch-triggered rebuilds.
Suggested change
| // Run once on start so the generated files are always fresh. | |
| runBuild(); | |
| // Run once on start so the generated files are always fresh. | |
| runBuild('initial startup'); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/theme/watch.mjs
Line: 16-17
Comment:
Calling `runBuild()` without an argument at startup will still print "token change detected" with the default. Pass a descriptive reason so the initial run is clearly distinguished from watch-triggered rebuilds.
```suggestion
// Run once on start so the generated files are always fresh.
runBuild('initial startup');
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+1
to
+9
| import { execSync } from 'node:child_process'; | ||
| /** | ||
| * Watches tokens.json and re-runs theme:build whenever it changes. | ||
| * Run via: pnpm run theme:watch | ||
| * Storybook HMR then picks up the updated theme.css automatically. | ||
| */ | ||
| import { watch } from 'node:fs'; | ||
| import { resolve, dirname } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; |
Contributor
There was a problem hiding this comment.
execSync import is placed before the JSDoc comment that describes the module, splitting the import block. Moving it with the other imports keeps the file header readable and consistent.
Suggested change
| import { execSync } from 'node:child_process'; | |
| /** | |
| * Watches tokens.json and re-runs theme:build whenever it changes. | |
| * Run via: pnpm run theme:watch | |
| * Storybook HMR then picks up the updated theme.css automatically. | |
| */ | |
| import { watch } from 'node:fs'; | |
| import { resolve, dirname } from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; | |
| /** | |
| * Watches tokens.json and re-runs theme:build whenever it changes. | |
| * Run via: pnpm run theme:watch | |
| * Storybook HMR then picks up the updated theme.css automatically. | |
| */ | |
| import { execSync } from 'node:child_process'; | |
| import { watch } from 'node:fs'; | |
| import { resolve, dirname } from 'node:path'; | |
| import { fileURLToPath } from 'node:url'; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/theme/watch.mjs
Line: 1-9
Comment:
The `execSync` import is placed before the JSDoc comment that describes the module, splitting the import block. Moving it with the other imports keeps the file header readable and consistent.
```suggestion
/**
* Watches tokens.json and re-runs theme:build whenever it changes.
* Run via: pnpm run theme:watch
* Storybook HMR then picks up the updated theme.css automatically.
*/
import { execSync } from 'node:child_process';
import { watch } from 'node:fs';
import { resolve, dirname } from 'node:path';
import { fileURLToPath } from 'node:url';
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+104
to
+119
| <div className="emlight flex-1 space-y-6 bg-background p-8"> | ||
| <p className="text-sm font-medium text-foreground">Light mode</p> | ||
| <div className="grid grid-cols-2 gap-3"> | ||
| {FAMILIES.map((f) => ( | ||
| <SurfaceCard key={f} family={f} /> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| <div className="emdark flex-1 space-y-6 bg-background p-8"> | ||
| <p className="text-sm font-medium text-foreground">Dark mode</p> | ||
| <div className="grid grid-cols-2 gap-3"> | ||
| {FAMILIES.map((f) => ( | ||
| <SurfaceCard key={f} family={f} /> | ||
| ))} | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
grid-cols-2, the last card renders as a lone item in its own row in both the light and dark panes. Using grid-cols-3 would keep the layout balanced (3+2) while still fitting in a half-screen column.
Suggested change
| <div className="emlight flex-1 space-y-6 bg-background p-8"> | |
| <p className="text-sm font-medium text-foreground">Light mode</p> | |
| <div className="grid grid-cols-2 gap-3"> | |
| {FAMILIES.map((f) => ( | |
| <SurfaceCard key={f} family={f} /> | |
| ))} | |
| </div> | |
| </div> | |
| <div className="emdark flex-1 space-y-6 bg-background p-8"> | |
| <p className="text-sm font-medium text-foreground">Dark mode</p> | |
| <div className="grid grid-cols-2 gap-3"> | |
| {FAMILIES.map((f) => ( | |
| <SurfaceCard key={f} family={f} /> | |
| ))} | |
| </div> | |
| </div> | |
| <div className="emlight flex-1 space-y-6 bg-background p-8"> | |
| <p className="text-sm font-medium text-foreground">Light mode</p> | |
| <div className="grid grid-cols-3 gap-3"> | |
| {FAMILIES.map((f) => ( | |
| <SurfaceCard key={f} family={f} /> | |
| ))} | |
| </div> | |
| </div> | |
| <div className="emdark flex-1 space-y-6 bg-background p-8"> | |
| <p className="text-sm font-medium text-foreground">Dark mode</p> | |
| <div className="grid grid-cols-3 gap-3"> | |
| {FAMILIES.map((f) => ( | |
| <SurfaceCard key={f} family={f} /> | |
| ))} | |
| </div> | |
| </div> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/stories/surfaces.stories.tsx
Line: 104-119
Comment:
With 5 families and `grid-cols-2`, the last card renders as a lone item in its own row in both the light and dark panes. Using `grid-cols-3` would keep the layout balanced (3+2) while still fitting in a half-screen column.
```suggestion
<div className="emlight flex-1 space-y-6 bg-background p-8">
<p className="text-sm font-medium text-foreground">Light mode</p>
<div className="grid grid-cols-3 gap-3">
{FAMILIES.map((f) => (
<SurfaceCard key={f} family={f} />
))}
</div>
</div>
<div className="emdark flex-1 space-y-6 bg-background p-8">
<p className="text-sm font-medium text-foreground">Dark mode</p>
<div className="grid grid-cols-3 gap-3">
{FAMILIES.map((f) => (
<SurfaceCard key={f} family={f} />
))}
</div>
</div>
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.