Skip to content

Commit 88db2a4

Browse files
lukemeliaclaude
andcommitted
Apply Burcu's design-review feedback to Adorn UI
Refactor the Adorn components/modifiers and their consumers to follow the design-system conventions: - Target DOM via a `data-adorn-context` attribute instead of the `.adorn-context` class - Convert px to rem across the Adorn CSS - Replace hardcoded teal/ink hex values with semantic tokens - Use the `cn` helper for conditional class names - Use `currentColor` for the themeable parts of inline SVGs - Use `--boxel-highlight` / `--boxel-highlight-hover` instead of `--boxel-teal` Consolidate the teal palette: `--boxel-dark-teal` becomes #00da9f (so `--boxel-highlight-hover` carries it), and add `--boxel-highlight-foreground` (#0a2e1c) as the readable foreground on highlight surfaces, following the established `<surface>` / `<surface>-foreground` convention. The new token is wired through the Adorn primitives, the card-search selection trigger, and the card-header utility-menu trigger (which previously duplicated the same pattern with hardcoded colors). Document these conventions and the content-tag `<template>`-detection hazards in a new gts-component-conventions skill and in AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e784a8d commit 88db2a4

12 files changed

Lines changed: 271 additions & 90 deletions

File tree

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
---
2+
name: gts-component-conventions
3+
description: Styling and authoring conventions for `.gts` components and their CSS in the host and boxel-ui packages, plus the content-tag `<template>`-detection hazards that silently break `.gts` parsing. Use whenever writing, reviewing, or refactoring a `.gts` component, a `<style scoped>` block, or a glimmer template — especially component-styling review passes ("apply these design-review notes", "clean up this component's CSS") and when a `.gts` file mysteriously fails to type-check or lints with phantom unused-import errors. Triggers on editing component markup/CSS, adding SVG icons, writing conditional class names, choosing colors or units, and any cascading "Cannot find name 'template'" or template-was-dropped symptom.
4+
---
5+
6+
# `.gts` Component Conventions
7+
8+
Two things live here:
9+
10+
1. **Design-review guidelines** (from Burcu) for component markup and CSS — apply when writing or reviewing any `.gts` component or `<style scoped>` block.
11+
2. **content-tag `<template>` hazards** — the parser gotchas that silently drop or mangle a template. Read part 2 the moment a `.gts` file fails to type-check or lints with phantom errors after an edit.
12+
13+
Root font-size is the browser default **16px**, so `1rem === 16px`. There is no `html { font-size }` override.
14+
15+
---
16+
17+
## Part 1 — Component & CSS guidelines
18+
19+
### 1. Target DOM elements with `data-*` attributes, not class names
20+
21+
Class names are a styling concern; JS that reaches into the DOM (`closest`, `querySelector`, `matches`, `hasAttribute`) should key off a `data-*` attribute so refactoring CSS classes never breaks behavior. Keep the class for the `<style>` selector **and** add a `data-*` marker for the JS hook.
22+
23+
```ts
24+
// Wrong — JS coupled to a styling class
25+
let marker = el.closest('.adorn-context');
26+
27+
// Right — class stays for CSS; data attribute is the JS contract
28+
let marker = el.closest('[data-adorn-context]');
29+
```
30+
31+
```hbs
32+
<div class='adorn-context' data-adorn-context ...attributes>
33+
```
34+
35+
`data-test-*` attributes (used by the test suite) already follow this — extend the same habit to runtime DOM lookups.
36+
37+
### 2. Use scalable units (`rem`) instead of `px`
38+
39+
Convert dimensional CSS values — `width`/`height`, `padding`, `margin`, `gap`, `border-radius`, `font-size`, `box-shadow` spreads, `clip-path` offsets, positioning insets — to `rem` (divide px by 16). Prefer the existing `--boxel-sp-*`, `--boxel-font-*`, and radius tokens when one fits.
40+
41+
```css
42+
gap: 0.3125rem; /* was 5px */
43+
padding: 0.1875rem 0.4375rem; /* was 3px 7px */
44+
font-size: 0.625rem; /* was 10px */
45+
box-shadow: 0 0 0 0.125rem var(--c); /* was 2px */
46+
```
47+
48+
**Leave as-is:** SVG-internal coordinates (`viewBox`, `cx`, `r`, `stroke-width`, path `d`), and JS pixel math against `getBoundingClientRect()` — those aren't CSS layout units. Hairline values like `letter-spacing: 0.5px` are fine to leave (converting gains nothing).
49+
50+
### 3. Save hardcoded colors as CSS variables
51+
52+
Never ship a raw hex/rgb in a component. If a color recurs across components, promote it to a shared token in `packages/boxel-ui/addon/src/styles/variables.css`; if it's truly local, define a component-scoped custom property. Falling back to another variable is fine (`var(--token, var(--other))`); a hardcoded literal fallback is not.
53+
54+
Reuse an existing semantic token before inventing a new one, and **name tokens by role, not by hue**. A color named for its appearance (`--boxel-teal-ink`, `--boxel-dark-teal`) is a palette primitive; a color named for its job (`--boxel-highlight`, `--boxel-highlight-hover`) is what components should reference.
55+
56+
For "readable text/icons on a colored surface," the codebase uses the pervasive **`<surface>` / `<surface>-foreground` pairing** (shadcn/Tailwind-style): `--foreground`, `--muted-foreground`, `--primary`/`--primary-foreground`, `--accent-foreground`, `--card-foreground`, plus component-local `--boxel-*-foreground`. The idiom is `color: var(--primary-foreground, var(--boxel-dark))`.
57+
58+
The adorn refactor therefore landed on existing/semantic tokens rather than hue-named ones:
59+
60+
- darker teal for hover/selected → `--boxel-highlight-hover` (resolves through `--boxel-dark-teal: #00da9f`). Don't add a parallel "teal-hover" variable.
61+
- dark foreground on a highlight surface → `--boxel-highlight-foreground: #0a2e1c` (the companion to `--boxel-highlight` / `--boxel-highlight-hover`, following the `-foreground` convention).
62+
63+
```css
64+
/* role-named, not hue-named or hardcoded */
65+
color: var(--boxel-highlight-foreground); /* was #0a2e1c */
66+
background-color: var(--boxel-highlight-hover); /* was #00da9f */
67+
```
68+
69+
### 4. Use the `cn` helper for conditional class names
70+
71+
Don't hand-concatenate classes with inline `{{if}}`/`{{unless}}` inside a class string. Use `cn` from `@cardstack/boxel-ui/helpers` — positional base classes, named boolean classes.
72+
73+
```hbs
74+
{{! Wrong }}
75+
<div class='adorn-label {{if @compact "compact"}} {{unless (has-block "dropdown") "no-menu"}}'>
76+
77+
{{! Right }}
78+
<div class={{cn 'adorn-label' compact=@compact no-menu=(unless (has-block 'dropdown') true)}}>
79+
```
80+
81+
`cn` emits the same space-separated string, so this is behavior-preserving — existing `data-test`/class selectors keep matching.
82+
83+
### 5. SVGs: keep them separate, stroke/fill with `currentColor`
84+
85+
- Factor SVG artwork into dedicated icon **components** (the repo convention — e.g. `selection-checkmark-icon.gts`) or `@cardstack/boxel-icons` / `@cardstack/boxel-ui/icons`, rather than duplicating raw `<svg>` markup. Ship compressed/optimized SVG.
86+
- Make the themeable parts `stroke='currentColor'` / `fill='currentColor'` so a parent can color the icon via `color:`. Parts that are intentionally a fixed brand color (e.g. a dark circle behind a themeable check) reference a token instead of a literal.
87+
88+
```hbs
89+
{{! themeable check follows the parent's color }}
90+
<path d='…' stroke='currentColor' />
91+
{{! fixed dark disc → token, not a hex literal }}
92+
<circle cx='7' cy='7' r='7' fill='var(--boxel-highlight-foreground)' />
93+
```
94+
95+
### 6. Use `--boxel-highlight`, not `--boxel-teal`, for the default highlight
96+
97+
`--boxel-highlight` resolves to `--boxel-teal` today, but it's the app-wide semantic token for the highlight accent. Referencing it keeps highlight color consistent and re-themeable across the app. (Same idea for `--boxel-highlight-hover`.)
98+
99+
```css
100+
/* prefer the semantic token, not the raw palette color */
101+
--adorn-accent-light: var(--boxel-highlight); /* not var(--boxel-teal) */
102+
background-color: var(--boxel-highlight); /* not var(--boxel-teal) */
103+
```
104+
105+
---
106+
107+
## Part 2 — content-tag `<template>` hazards
108+
109+
`content-tag` (the preprocessor glint and `ember-eslint-parser` use to parse `.gts`) has JavaScript-lexer bugs that make it lose track of `<template>` tags. When that happens the template is silently dropped or misparsed — and the symptom is **not** where the bad character is. AGENTS.md (§ "`.gts` file gotcha") is the canonical list; the known triggers:
110+
111+
**1. Backticks inside a regex literal** — mistaken for template-literal delimiters.
112+
113+
```ts
114+
.replace(/`([^`]+)`/g, '$1') // BROKEN
115+
const INLINE_CODE_RE = new RegExp('`([^`]+)`', 'g'); // FIX
116+
```
117+
118+
**2. `!/regex/` (negation before a regex literal)** — the `/` after `!` is misread.
119+
120+
```ts
121+
lines.some((line) => !/^\s*#/.test(line)); // BROKEN
122+
const HEADING_RE = /^\s*#/; // FIX — extract to a const
123+
lines.some((line) => !HEADING_RE.test(line));
124+
```
125+
126+
**3. A backtick-wrapped bracket token inside the _template body_** — e.g. a `<style>` CSS comment.
127+
128+
```hbs
129+
<template>
130+
<style scoped>
131+
/* BROKEN: backtick-wrapped `[data-adorn-context]` here drops the template */
132+
/* FIX: drop the backticks or the brackets in template-region comments */
133+
</style>
134+
</template>
135+
```
136+
137+
The identical text in a `//` comment _outside_ `<template>…</template>` is harmless — content-tag only runs its template-mode lexer between the tags. So keep backtick-wrapped selectors like `` `[data-foo]` `` out of comments inside the template; describe the marker in a regular JS comment above the component instead.
138+
139+
### Recognizing it
140+
141+
- **Outside-template triggers (1 & 2):** cascading TypeScript errors beginning with `Cannot find name 'template'` at the first `<template>` in the file.
142+
- **Inside-template trigger (3):** the template body is silently dropped, so **type-check may still pass** while ESLint reports phantom `no-unused-vars` on imports/consts that the template references (e.g. `hash`, a yielded const). **ESLint is the canary** here — if a refactor that only touched a `<template>`/`<style>` block suddenly makes prior imports "unused," suspect a swallowed template before you touch the imports.
143+
144+
### Bisecting
145+
146+
Revert to the last-good file and re-apply changes one hunk at a time, running `npx eslint <file>` between each, until the phantom errors reappear. The offending hunk is almost always a comment or string edit inside the `<template>` block, not a code change.

AGENTS.md

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ Scopes are allowed: `feat(profile): …`. Other monorepo packages are unaffected
196196

197197
## `.gts` file gotcha: regex literals can break content-tag
198198

199-
The `content-tag` preprocessor (used by glint and ember-eslint-parser to parse `.gts` files) has bugs in its JavaScript lexer that cause it to misparse certain regex literals. When this happens, it fails to recognize `<template>` tags later in the file, producing cascading parse errors. Two known triggers:
199+
The `content-tag` preprocessor (used by glint and ember-eslint-parser to parse `.gts` files) has bugs in its JavaScript lexer that cause it to misparse certain regex literals. When this happens, it fails to recognize `<template>` tags later in the file, producing cascading parse errors. Three known triggers:
200200

201201
**1. Backticks inside regex literals** — content-tag mistakes them for template literal delimiters:
202202

@@ -220,6 +220,18 @@ const HEADING_RE = /^\s*#{1,6}\s+/;
220220
lines.some((line) => !HEADING_RE.test(line));
221221
```
222222

223+
**3. Backtick-wrapped bracket token inside the `<template>` body** — a comment _inside_ the template (e.g. a `<style scoped>` CSS comment) that contains a backtick-wrapped selector like `` `[data-foo]` `` drops the template. The same text in a `//` comment _outside_ `<template>…</template>` is harmless; content-tag only runs its template-mode lexer between the tags.
224+
225+
```hbs
226+
<template>
227+
<style scoped>
228+
{{! BROKEN: backtick-wrapped `[data-foo]` in a template-region comment }}{{! FIX: drop the backticks or the brackets, or move the note to a JS comment above the component }}
229+
</style>
230+
</template>
231+
```
232+
233+
Symptom differs from triggers 1–2: the template body is silently dropped, so type-check can still pass while ESLint reports phantom `no-unused-vars` on imports/consts the template references (e.g. `hash`, yielded consts). ESLint is the canary.
234+
223235
## Base realm imports
224236

225237
- Only card definitions (files run through the card loader) can use static ESM imports from `https://cardstack.com/base/*`. Host-side modules must load the module at runtime via `loader.import(`${baseRealm.url}...`)`. Static value imports from the HTTPS specifier inside host code trigger build-time `webpackMissingModule` failures. Type imports are OK using static ESM syntax.

packages/boxel-ui/addon/src/components/card-header/index.gts

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -139,10 +139,15 @@ export default class CardHeader extends Component<Signature> {
139139
fill='none'
140140
aria-hidden='true'
141141
>
142-
<circle cx='7' cy='7' r='7' fill='#0a2e1c' />
142+
<circle
143+
cx='7'
144+
cy='7'
145+
r='7'
146+
fill='var(--boxel-highlight-foreground)'
147+
/>
143148
<path
144149
d='M3.5 7.5L5.5 9.5L10.5 4.5'
145-
stroke='var(--boxel-teal)'
150+
stroke='var(--boxel-highlight)'
146151
stroke-width='1.5'
147152
stroke-linecap='round'
148153
stroke-linejoin='round'
@@ -315,7 +320,7 @@ export default class CardHeader extends Component<Signature> {
315320
only the pencil lights up (see submode-layout CSS). */
316321
header.is-editing {
317322
background-color: var(--boxel-highlight);
318-
color: var(--boxel-dark);
323+
color: var(--boxel-highlight-foreground);
319324
}
320325
.boxel-card-header__inner {
321326
width: 100%;
@@ -427,7 +432,7 @@ export default class CardHeader extends Component<Signature> {
427432
presence doesn't widen the actions column, which would otherwise
428433
shift the centered card title off-center. */
429434
.utility-menu-positioner {
430-
--utility-menu-trigger-height: 26px;
435+
--utility-menu-trigger-height: 1.625rem;
431436
position: absolute;
432437
right: calc(100% + var(--boxel-sp-5xs));
433438
top: 50%;
@@ -446,19 +451,19 @@ export default class CardHeader extends Component<Signature> {
446451
width: max-content;
447452
padding: 0 var(--boxel-sp-xs);
448453
border: none;
449-
border-radius: 6px;
450-
background-color: var(--boxel-teal);
451-
color: var(--boxel-dark);
454+
border-radius: 0.375rem;
455+
background-color: var(--boxel-highlight);
456+
color: var(--boxel-highlight-foreground);
452457
font: 700 var(--boxel-font-sm);
453458
cursor: pointer;
454459
}
455460
.utility-menu-trigger:hover:not(:disabled),
456461
.utility-menu-trigger:focus-visible:not(:disabled) {
457-
background-color: #00da9f;
462+
background-color: var(--boxel-highlight-hover);
458463
}
459464
.utility-menu-trigger-icon {
460-
width: 14px;
461-
height: 14px;
465+
width: 0.875rem;
466+
height: 0.875rem;
462467
flex-shrink: 0;
463468
}
464469
.utility-menu-trigger-text {

packages/boxel-ui/addon/src/styles/variables.css

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@
170170

171171
--boxel-cyan: #00ebe5;
172172
--boxel-teal: #00ffba;
173-
--boxel-dark-teal: #00ebac;
173+
--boxel-dark-teal: #00da9f; /* darker teal; feeds --boxel-highlight-hover */
174174
--boxel-red: #ff5050;
175175
--boxel-pink: #ff009d;
176176
--boxel-orange: #ff7f00;
@@ -196,6 +196,7 @@
196196

197197
--boxel-highlight: var(--boxel-teal);
198198
--boxel-highlight-hover: var(--boxel-dark-teal);
199+
--boxel-highlight-foreground: #0a2e1c; /* readable dark-green foreground for text/icons on a --boxel-highlight surface */
199200
--boxel-danger: var(
200201
--boxel-red
201202
); /* formerly boxel-error-100: the latest design docs (8/16/2023) use this color for danger instead */

packages/host/app/components/adorn/adorn-context.gts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,10 @@ import type { ModifierLike } from '@glint/template';
5757
// wrapper. AdornContext renders as `display: contents`, so its own
5858
// rect is empty — its parent is the element the consumer mounted it
5959
// inside, which is the region we want. Wired into the yielded
60-
// `positionLabel` modifier so the `.adorn-context` marker stays an
61-
// implementation detail of this component.
60+
// `positionLabel` modifier so the `[data-adorn-context]` marker stays
61+
// an implementation detail of this component.
6262
function getBoundaryElement(el: HTMLElement): HTMLElement | null {
63-
let marker = el.closest<HTMLElement>('.adorn-context');
63+
let marker = el.closest<HTMLElement>('[data-adorn-context]');
6464
return marker?.parentElement ?? null;
6565
}
6666

@@ -85,7 +85,7 @@ interface AdornContextSignature {
8585
}
8686

8787
const AdornContext: TemplateOnlyComponent<AdornContextSignature> = <template>
88-
<div class='adorn-context' ...attributes>
88+
<div class='adorn-context' data-adorn-context ...attributes>
8989
{{yield (hash strokeClass='adorn-stroke' positionLabel=positionLabel)}}
9090
</div>
9191
<style scoped>
@@ -99,11 +99,11 @@ const AdornContext: TemplateOnlyComponent<AdornContextSignature> = <template>
9999
display: contents;
100100
101101
/* Token definitions live with the context, not in the global
102-
stylesheet. --boxel-teal is the light accent shipped by
103-
boxel-ui; the darker accent is exclusive to the Adorn
104-
treatment and used when a selected card is hovered. */
105-
--adorn-accent-light: var(--boxel-teal);
106-
--adorn-accent: #00da9f;
102+
stylesheet. --boxel-highlight is the app-wide highlight accent
103+
shipped by boxel-ui; --boxel-highlight-hover is its darker
104+
companion, used here when a selected card is hovered. */
105+
--adorn-accent-light: var(--boxel-highlight);
106+
--adorn-accent: var(--boxel-highlight-hover);
107107
}
108108
/* Stroke utility. The consumer applies `.adorn-stroke` to
109109
whichever descendant should carry the outline (typically the
@@ -115,14 +115,14 @@ const AdornContext: TemplateOnlyComponent<AdornContextSignature> = <template>
115115
element it's rendered inside. */
116116
.adorn-context :deep(.adorn-stroke:hover:not(.selected)),
117117
.adorn-context :deep(.adorn-stroke.hovered:not(.selected)) {
118-
box-shadow: 0 0 0 2px var(--adorn-accent-light);
118+
box-shadow: 0 0 0 0.125rem var(--adorn-accent-light);
119119
}
120120
.adorn-context :deep(.adorn-stroke.selected) {
121-
box-shadow: 0 0 0 4px var(--adorn-accent-light);
121+
box-shadow: 0 0 0 0.25rem var(--adorn-accent-light);
122122
}
123123
.adorn-context :deep(.adorn-stroke.selected:hover),
124124
.adorn-context :deep(.adorn-stroke.selected.hovered) {
125-
box-shadow: 0 0 0 4px var(--adorn-accent);
125+
box-shadow: 0 0 0 0.25rem var(--adorn-accent);
126126
--adorn-label-bg: var(--adorn-accent);
127127
}
128128
</style>

0 commit comments

Comments
 (0)