Repository navigation
Conversation
Fixes and cleanups from the boxel-ui-guidelines UI review of Table, Pagination, Breadcrumb, StepList, ProgressBar, Meter, Token, Avatar, Alert, TableOfContents, KeyValue and EmptyState. Behavior: - Table: a --pretui-table-max-height knob bounds the wrapper so the sticky header sticks inside it. - Pagination: the current page keeps its selected fill under hover; page buttons carry an aria-label; press feedback respects prefers-reduced-motion. - ProgressBar: the usage page offers --info and --attention, which the theme contract defines, and progress motion respects reduced motion. - Token: keepHue is replaced by keepStyle; the default size is a contract font size; sizes map onto the Boxel font-size ladder. - Meter: bar heights are written in rem. - Avatar: initials carry role="img" with the name; a photo drops the root label and gets a neutral ring; the size default is declared once. - Alert: tone glyphs are boxel-icons; the info tone paints --info; text uses the tone's -ink token. - TableOfContents: the row is built once for link and button modes. - KeyValue and EmptyState: contract tokens, the eyebrow role, baseline alignment, and a real heading and paragraph in EmptyState. Hygiene across the components: contract tokens are read without fallbacks, hardcoded px move onto the spacing, radius and font-size ladders, plain colors use background-color, text glyphs and inline SVGs become icons, and docs use American spelling. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…lert and Pagination output The Meter writes rem heights, an Avatar photo that loads keeps the photo markup, each Alert tone paints its own icon, and every page button carries a label. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The EmptyState heading level is an open decision, so the title stays a plain element until it is made. Pagination keeps no extra arrow mirroring or cursor rule, the Alert icon test no longer assumes the icon's class name, and ink.test.gts is left as it was. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Both components are being reworked in open changes that touch the same files, so this change keeps them as they are. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The Meter heights are rem, and the test host serves every icon module as one placeholder, so the Alert test checks for an svg per tone and not which icon it is. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91996bef53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <div class='pretui-empty-msg'>{{yield}}</div> | ||
| {{else if @message}} | ||
| <div class='pretui-empty-msg'>{{@message}}</div> | ||
| <p class='pretui-empty-msg'>{{@message}}</p> |
There was a problem hiding this comment.
When EmptyState receives @message, changing this element from a div to a p introduces the browser's default block margins because .pretui-empty-msg does not reset margin. These margins do not collapse inside the grid, so the common argument-based form gets substantially more vertical spacing than the equivalent default-block form and pushes any actions downward; add margin: 0 to the shared message style.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Left as is. CardContainer already zeroes p margins, so a margin: 0 on .pretui-empty-msg would repeat what the container provides, and the UI review procedure treats that repeat as dead weight. The @message branch renders a p on purpose, and the default-block branch stays a div so a caller's block markup is not wrapped in a paragraph. Outside a CardContainer the browser's paragraph margins would apply.
There was a problem hiding this comment.
[Claude Code 🤖] Correction to my reply above: fixed in ab5f4f188d. .pretui-empty-msg sets margin: 0, because the kit's components render outside a CardContainer too, where the paragraph margin would show.
| <template> | ||
| <nav class='pretui-pagination' aria-label='Pagination' data-test-pretui-pagination ...attributes> | ||
| <button type='button' class='pretui-page' aria-disabled={{if this.atStart 'true'}} aria-label='Previous' {{on 'click' this.prev}}>‹</button> | ||
| <button type='button' class='pretui-page' aria-disabled={{if this.atStart 'true'}} aria-label='Previous' {{on 'click' this.prev}}><ChevronLeft class='pretui-chevron' width='12' height='12' aria-hidden='true' /></button> |
There was a problem hiding this comment.
When this component is rendered under dir="rtl", the new fixed SVGs keep pointing left for Previous and right for Next, whereas the replaced Unicode glyphs were bidi-mirrored. This makes both controls visually indicate the opposite navigation direction for RTL users; mirror the icons with :dir(rtl) or choose their direction based on the computed writing direction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 8fa535f6fb. The arrows are now mirrored under a right-to-left writing direction with .pretui-chevron:dir(rtl) { scale: -1 1 }, so Previous and Next point the way the bidi-mirrored glyphs did. pagination.md says so.
…ing direction The arrows are fixed SVGs, while the glyphs they replace were bidi-mirrored, so Previous and Next pointed the wrong way in RTL. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
| <div class='pretui-empty-msg'>{{yield}}</div> | ||
| {{else if @message}} | ||
| <div class='pretui-empty-msg'>{{@message}}</div> | ||
| <p class='pretui-empty-msg'>{{@message}}</p> |
There was a problem hiding this comment.
[Claude Code 🤖] @message now renders in a <p>, but .pretui-empty-msg never resets margin, so the paragraph keeps the UA's 1em top/bottom margin on top of the grid gap. Every @message empty state gains about 12px of extra space above and below the message, and the block (div) and arg (p) variants no longer line up. The Alert title got margin: 0 for the same div-to-p change; this one needs it too.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. .pretui-empty-msg now sets margin: 0, so the paragraph (@message) and the block form line up and the message adds no extra space. This also corrects my earlier reply on the same point in the other thread.
| gap: var(--boxel-sp-xs); | ||
| padding: var(--boxel-sp-2xs) var(--boxel-sp-xs); | ||
| border-radius: var(--boxel-border-radius); | ||
| background-color: color-mix(in oklch, var(--pretui-alert-hue) var(--pretui-alert-mix), var(--card)); |
There was a problem hiding this comment.
[Claude Code 🤖] The var(--pretui-alert-hue, var(--chart-1)) fallbacks are gone, so the tint, hairline, ink and disc now rely entirely on the inline style from hueStyle. A caller that passes style=... (through ...attributes, which replaces the component's own style) unsets --pretui-alert-hue, --pretui-alert-ink and --pretui-alert-on-hue. Each color-mix then becomes invalid at computed-value time: the background goes transparent, the text inherits and the glyph disc disappears. Before this change the alert fell back to --chart-1 and kept its styling.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. The three hue properties are now also written by keepStyle, the way Avatar keeps its own, so a caller's style no longer drops the tint, ink or glyph disc. The reads stay bare. A new test passes style and checks the properties and the caller's declaration are both kept.
| <style scoped> | ||
| @layer PretComponent { | ||
| .pretui-alert { | ||
| --pretui-alert-mix: 20%; |
There was a problem hiding this comment.
[Claude Code 🤖] Alert no longer reads --pretui-chip-mix, and it declares --pretui-alert-mix: 20% on .pretui-alert itself, so a season or ancestor can't retune it. chip.md ("shared with Alert, so a banner and a chip about the same thing tint identically") and result.md still promise the shared token. A theme that raises --pretui-chip-mix now tints chips and results but leaves alerts at 20%. Either read var(--pretui-alert-mix, var(--pretui-chip-mix, 20%)) without declaring it on the root, or update the Chip and Result docs.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] @burieberry @habdelra this one needs a decision, so I left the code as it is. The review of the merged Alert asked not to read --pretui-chip-mix (a Pret UI season variable), so Alert declares its own --pretui-alert-mix: 20%. chip.md and result.md still say the tint strength is shared with Alert. Two ways: (a) keep Alert independent and update the Chip and Result docs; (b) read var(--pretui-alert-mix, var(--pretui-chip-mix, 20%)) so the shared knob keeps working. Which one?
There was a problem hiding this comment.
[Claude Code 🤖] (a). --pretui-chip-mix is a Pret season variable, and nothing in boxel-ui or the host declares it, so in Boxel the "shared" knob never reached Alert or Chip. Keep --pretui-alert-mix as Alert's own, and drop the "shared with Alert" claims from chip.md (the mix-ratio paragraph and Theming) and result.md (Theming). Chip, Result, ErrorSummary, FormSection and TreeSelect still read --pretui-chip-mix with literal fallbacks; that is a follow-up for those components, not this change.
There was a problem hiding this comment.
[Claude Code 🤖] Done in 9d5fa50713, option (a). Alert keeps its own --pretui-alert-mix, and the "shared with Alert" claims are removed from chip.md (the mix-ratio paragraph and Theming) and from the Theming section of result.md.
| d='M3.2 2.1 7.6 5 3.2 7.9Z' | ||
| fill='currentColor' | ||
| /></svg> | ||
| <PlayerPlayIcon width='9' height='9' /> |
There was a problem hiding this comment.
[Claude Code 🤖] These boxel-icons are 24-unit outline icons with stroke-width='2', so at width='9' the strokes render at about 0.75px. The replaced inline SVGs drew 1.44px strokes, and the running marker was a filled triangle; PlayerPlayIcon is a hollow outline. In the 1.125rem marker (and the track variant's caption), complete/error/blocked/running become faint hairlines that are hard to tell apart at 1x DPR. Pass a heavier stroke-width (for example stroke-width='3') and use fill='currentColor' for the play icon, or keep the tuned paths.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. The four step icons now use stroke-width='4' (about 1.5px at 9px, matching the strokes of the inline SVGs they replaced), and the running icon is filled with currentColor.
| warning: '!', | ||
| danger: '✕', | ||
| const ALERT_ICONS = { | ||
| info: InfoIcon, |
There was a problem hiding this comment.
[Claude Code 🤖] InfoIcon draws its own outer circle (<circle r=10> plus the i), and here it sits inside the 1rem filled disc. The info tone shows a ring inside a disc, while success, warning and danger show bare glyphs (check, triangle, x). The icon also loses most of its size to the inner ring at 12px. Use a bare "i" glyph, or drop the disc for info.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. The info tone uses the bare i icon (info-small), so there is no ring inside the disc.
| {{else}} | ||
| <button | ||
| type='button' | ||
| {{#let (element this.rowTag) as |Row|}} |
There was a problem hiding this comment.
[Claude Code 🤖] element from boxel-ui returns a classic @ember/component subclass, so each TOC row is now a classic component instance. Each one stamps an id="emberNNN" and class="ember-view" onto the link/button and costs more than a plain element per row (TOCs can be long). The previous {{#if this.links}} with two plain elements was cheaper; if the goal is deduping the body, a shared inner block or template-only component avoids the classic-component overhead.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. The row is a small template-only component that renders the a or the button, with the shared attributes passed once through ...attributes. There is no classic component per row, and the element helper is gone.
| transition-timing-function: var(--pretui-ease-snap, ease); | ||
| } | ||
| /* button mode: the same row, without the button's own chrome */ | ||
| button.pretui-toc-link { |
There was a problem hiding this comment.
[Claude Code 🤖] Button-mode rows lost cursor: pointer, and font: inherit was narrowed to family/size/line-height. A @links={{false}} row now shows the arrow cursor while the <a> rows (and the Pagination buttons in this same PR) show the pointer. Button rows also stop inheriting font-weight, font-style and letter-spacing from the TOC (button UA styles reset them), so a caller's weight on .pretui-toc reaches link mode but not button mode.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Half fixed in ab5f4f188d: button rows now inherit font-weight, font-style and letter-spacing. The cursor I left as it is, because it needs a decision. @burieberry the review of the merged component asked to drop cursor: pointer since global.css already sets it on button:hover:not(:disabled). Is that stylesheet always loaded wherever TableOfContents renders? If not, the cursor should come back on the button rows.
There was a problem hiding this comment.
[Claude Code 🤖] Yes. The host's app.ts imports @cardstack/boxel-ui/styles/global.css, and every Pret UI consumer (base, catalog, experiments-realm) renders through the host, prerender included. Its cursor: pointer on button:hover:not(:disabled) covers the button rows, so leave it deleted. Pagination should drop its own for the same reason; see the comment on .pretui-page[data-state='active'].
There was a problem hiding this comment.
[Claude Code 🤖] Thanks. cursor: pointer stays deleted on the TOC rows, and Pagination dropped its own in 9d5fa50713.
| color: var(--muted-foreground); | ||
| cursor: pointer; | ||
| font: inherit; | ||
| font-family: inherit; |
There was a problem hiding this comment.
[Claude Code 🤖] Swapping font: inherit for four longhands leaves font-style, font-stretch and the other font-variant-* at the button UA defaults rather than inheriting them. A pagination placed in italic or condensed text now renders its page numbers upright and normal width while the ellipsis span (not a button) follows the parent. font: inherit followed by the font-variant-numeric override did the same job in one line.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. .pretui-page now also inherits font-style, font-stretch and font-variant, before the tabular-nums override. I kept the individual properties, since the UI review asked to split the font shorthand.
| color: var(--subtle-foreground); | ||
| cursor: default; | ||
| } | ||
| /* SVG icons do not mirror with the writing direction the way the |
There was a problem hiding this comment.
[Claude Code 🤖] This comment ("SVG icons do not mirror with the writing direction...") sits above .pretui-gap, which has nothing to do with icons. It is a leftover duplicate of the .pretui-chevron:dir(rtl) comment above and contradicts that rule, since the icons now do mirror. Remove it.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. The stray comment is removed.
| That last-item rule is the whole opinion. The current location is not a link even if you gave it one, because linking to where you already are is a dead affordance — and making it structurally impossible means no call site has to remember. The `<span>` branch exists for intermediate ancestors that are real but unreachable (a folder you cannot open, a realm you cannot read), which is a genuine Boxel case. | ||
|
|
||
| Separators are `/` characters rendered between items, styled `--ink-3`. | ||
| Separators are `/` characters rendered between items, styled `--subtle-foreground`. Each carries `data-test-pretui-breadcrumb-sep`. |
There was a problem hiding this comment.
[Claude Code 🤖] The component doc now advertises data-test-pretui-breadcrumb-sep as part of the separator's description. AGENTS.md says: "data-test-* attributes are stripped from production builds. Never use them for runtime behavior or styling in app code." Documenting it next to the theming notes invites callers to hook styling or behavior onto an attribute that disappears in production. Keep it out of the user-facing doc, or name the production hook (.sep) instead.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in ab5f4f188d. breadcrumb.md no longer names the data-test-* attribute; the test keeps using it.
- EmptyState: .pretui-empty-msg resets margin so the paragraph and block forms match. - Alert: the hue, ink and on-hue properties are kept on top of a caller's style; the info tone uses a bare i glyph. - StepList: heavier icon strokes and a filled play icon. - TableOfContents: one template-only link/button component instead of a classic component per row; button rows inherit weight, style and letter-spacing. - Pagination: every inherited font property, and no stray comment. - Breadcrumb: the doc no longer names a data-test attribute. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
burieberry
left a comment
burieberry
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review runs the boxel-ui-guidelines UI review procedure (contract tokens, guaranteed pairs, typography roles, ladders, markup) and an interface pass (accessibility, layout, writing, type, color, polish) over all 37 files, reading each whole file. Contrast is computed from the theme.css and variables.css values against the boxel default theme in light and dark. Nothing was rendered, so the sticky header, the right-to-left arrows, icon stroke weight and hit sizes are unverified.
One blocking regression: the danger Alert's text falls below 4.5:1 in the light default theme. Everything else is small, plus a merge conflict.
- Danger Alert text contrast (blocking): see the comment on
colorinalert.gts. - The current page keeps a pointer cursor: see the comment on
.pretui-page[data-state='active']inpagination.gts. - ProgressBar doc: a sentence about the doc's own upkeep, and a stale 4px. See
progress-bar.md. --font-serifclaim: seeempty-state.md.- Merge conflict in
table.md.mainremoved the sentence sayingCardContainerpublishes the label role and the caption's letter-spacing, because the role defaults now live intheme.css. Keep that deletion when resolving, and keep the new sticky-header text.
Both questions addressed to me in the threads are answered there.
Pre-existing in the touched files, not caused by this change. These are follow-ups unless you'd rather fold them in. Any you defer needs a ticket linked from the description.
step-list.gts: the in-progress state reads--pretui-infofour times, which nothing in Boxel defines, so it paints--primary. The play icon is then teal on a 16% teal tint: 1.25:1 in light, against the 3:1 non-text minimum. Use--infofor the tint, ring and bar, and--info-inkfor the marker (9.86:1). The comment above the rule ("the primary hue filled solid, like 'current'") doesn't match the tint either way.letter-spacing: var(--track-ui, 0.01em)on.pretui-stepalso reads a season variable; usevar(--boxel-ui-label-letter-spacing).pagination.gts: the active page'sbox-shadow: var(--pretui-shadow-hairline, 0 0 0 1px var(--border))reads a season variable through a fallback. Use0 0 0 1px var(--border), which is the change the TableOfContents usage page already makes here.empty-state.usage.gts: both<Button @variant='secondary'>pass the deprecated@variant. Use@tone='neutral' @appearance='outlined'.table-of-contents.usage.gts:.toc-docpaintsbackground: var(--card)without its ink. Usebackground-color: var(--card); color: var(--card-foreground);, then dropcolor: var(--foreground)from.toc-h.- Hover on Pagination buttons and TableOfContents rows isn't gated on
(hover: hover)the way Table's is, so a tapped row keeps the hover fill on touch screens. - TableOfContents positions the rail, marker and indent with physical
leftandpadding-left, so underdir="rtl"the rail stays on the left. Pagination's arrows mirror; this doesn't.
| padding: var(--boxel-sp-2xs) var(--boxel-sp-xs); | ||
| border-radius: var(--boxel-border-radius); | ||
| background-color: color-mix(in oklch, var(--pretui-alert-hue) var(--pretui-alert-mix), var(--card)); | ||
| color: var(--pretui-alert-ink); |
There was a problem hiding this comment.
[Claude Code 🤖] Blocking: the danger Alert's text fails contrast in the light default theme. theme.css sets --destructive-ink to the fill itself (#dc0202) at :root, so in any card without its own Theme, the danger title and body measure 3.93:1 on the 20% tint (#ffd7d1). That is under the 4.5:1 that 12px text needs, and the old color mix gave 9.6:1. The -ink tokens are only guaranteed on --background, --card and --muted, and the tint is none of those.
--foreground measures at least 15.9:1 on all four tints in light and at least 9.5:1 in dark, so paint the text with it and let the tint, hairline and disc carry the tone:
| color: var(--pretui-alert-ink); | |
| color: var(--foreground); |
Then drop ink from ALERT_COLORS, hueStyle, keptStyle, the caller-style test, and the Theming sections of alert.md and callout.md. Regression; blocks.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 9d5fa50713. The Alert text is now var(--foreground), and ink is gone from ALERT_COLORS, hueStyle, keptStyle, the caller-style test, and the Theming sections of alert.md and callout.md.
| background-color: var(--selected); | ||
| color: var(--primary-ink); |
There was a problem hiding this comment.
[Claude Code 🤖] The current page still shows a pointer cursor, though pressing it does nothing. Add cursor: default; to this rule. Also delete cursor: pointer from .pretui-page: boxel-ui's global.css already sets it on button:hover:not(:disabled) in @layer reset, and @layer PretComponent overrides that layer, so the cursor: default rules here still win. TableOfContents dropped its own for the same reason. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 9d5fa50713. .pretui-page no longer sets cursor: pointer, and .pretui-page[data-state='active'] sets cursor: default.
|
|
||
| - **No announcement on change.** A progress bar that advances silently is correct for a fast operation and unhelpful for a slow one; there is no live region and no hook for one. | ||
| - **The fill fails WCAG 1.4.11 Non-text Contrast (3:1) against its track in the shipped light seasons.** The fill (`--pretui-progress-hue`, default `--primary`) sits on `--inset` with no border, and stepped mode tells lit from unlit segments by that colour alone. Measured against light `--inset`: the default `--primary` is 1.20:1 in SS26 (3.37:1 in AW26, 4.84:1 in SS27); `--warning` is 2.14, 1.58 and 2.01:1; AW26's `--pretui-attention` is 1.45:1 and its `--destructive` 2.90:1; SS26's `--success` and `--pretui-info` are 2.55 and 2.70:1. Every dark-mode pair passes, at 5.20:1 or higher. | ||
| - **The fill fails WCAG 1.4.11 Non-text Contrast (3:1) against its track in the boxel light default theme.** The fill (`--pretui-progress-hue`, default `--primary`) sits on `--inset` with no border, and stepped mode tells lit from unlit segments by that color alone. In the boxel light default, `--primary` (`--boxel-teal`, `#00ffba`) on `--inset` (`#f5f5f5`) is 1.2:1, against the 3:1 that 1.4.11 requires; a lighter track cannot fix it (teal on white is 1.31:1), so a light-theme `3 / 6` bar reads as empty. The dark default passes (10.2:1 on `#302d3b`). The fill has to darken in light mode; that fix is tracked separately and this paragraph should be updated when it lands. |
There was a problem hiding this comment.
[Claude Code 🤖] The last sentence describes upkeep of the doc itself, and goes stale the moment the fill changes. End the bullet at "The fill has to darken in light mode." The next line also still says "The 4px bar height"; make it 0.25rem to match the rest of the doc. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 9d5fa50713. The bullet ends at "The fill has to darken in light mode.", and the next line reads "The 0.25rem bar height".
| A season **must** define `--font-serif`; it is used almost nowhere else, so a season that omits it falls back to Georgia and the one moment of typographic voice in the kit lands on a system font. A season must also keep `--canvas` distinguishable from `--card`, or the empty state stops reading as a recess and the whole effect flattens. | ||
| The message and separator are set in `--foreground`, at the smaller size, because the contract guarantees `--muted-foreground` only on `--background`, `--card` and `--muted`, not on the neutral `--canvas`. | ||
|
|
||
| A theme **must** define `--font-serif`; it is used almost nowhere else, so a season that omits it falls back to Georgia and the one moment of typographic voice in the kit lands on a system font. A theme must also keep `--canvas` distinguishable from `--card`, or the empty state stops reading as a recess and the whole effect flattens. |
There was a problem hiding this comment.
[Claude Code 🤖] --font-serif has a theme.css default (IBM Plex Serif), so a theme that doesn't set it gets that, not Georgia. The rewritten sentence also still says "a season". Non-blocking.
| A theme **must** define `--font-serif`; it is used almost nowhere else, so a season that omits it falls back to Georgia and the one moment of typographic voice in the kit lands on a system font. A theme must also keep `--canvas` distinguishable from `--card`, or the empty state stops reading as a recess and the whole effect flattens. | |
| The title uses the theme's `--font-serif` (IBM Plex Serif by default), which is used almost nowhere else in the kit, so it is the component's one moment of typographic voice. A theme must also keep `--canvas` distinguishable from `--card`, or the empty state stops reading as a recess and the whole effect flattens. |
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 9d5fa50713 with your suggested wording.
…the-ui-review-follow-ups-left-on-the-merged-56 # Conflicts: # packages/pretui/components/table.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[Claude Code 🤖] @burieberry all of your review is addressed in The pre-existing items from your summary are fixed in this PR too:
The |
3 participants
Background and Goal
Where to start
table.gts(height knob for the sticky header),pagination.gts(hover on the current page, page labels),avatar.gts(role, photo ring, size default),empty-state.gtsandalert.gts(markup),table-of-contents.gts(one row element for both modes).background-colorinstead of thebackgroundshorthand, icons instead of text glyphs, American spellings. The docs and usage pages follow each change.Where each file comes from
33 of the 37 files come straight from the review's comments on the merged components. The other 4 are knock-on edits.
table.gts,table.mdpagination,breadcrumbandstep-list(code, docs, tests;step-listalso its usage page)meter(code, docs),progress-bar(code, docs, test, usage)avatar(code, docs, test, usage)alert(code, docs, test, usage)table-of-contents(code, docs, usage)empty-state(code, docs, test, usage)The knock-on files:
ink.test.gts(the Meter height test now expects rem),callout.md(the alias doc for Alert, which followsalert.md),error-summary.md(it described Alert's text glyphs) andreading-listing.test.gts(it picked page buttons by "noaria-label", and every page button now has one).Key decisions and non-obvious mechanics
overflow-x: automakes the wrapper the header's scroll container, so the header sticks only once the wrapper itself scrolls vertically.--pretui-table-max-heightbounds the wrapper; unset it isnoneand the table is as tall as its rows.@messagein ap; the title stays a plain element until the heading element is decided (see below).--info. Text is--foreground, because the-inktokens are only guaranteed on--background,--cardand--muted, and the default--destructive-inkfalls under 4.5:1 on the danger tint. The tint strength is Alert's own--pretui-alert-mix.Not in this PR
IconButtonandButton, Avatar@sizeon the shared scale, Button@sizeon the same font-size ladder as Token, EmptyState heading element, Alert body element): https://linear.app/cardstack/issue/CS-13618role="status": https://linear.app/cardstack/issue/CS-13619--pretui-chip-mix/--pretui-ink-mixseason variables: https://linear.app/cardstack/issue/CS-13657🤖 Generated with Claude Code