Cloonar/coding-lab

Fork 0

fix(web): section cards own their head/hint spacing, extract SectionCard #277

Merged

dominik.polakovics merged 1 commit from afk/275 into main

2026-08-04 15:55:48 +02:00

dominik.polakovics commented

2026-08-04 14:40:11 +02:00

Owner

Closes #275

The bug

At empty states the head-row action button could print on top of the hint below it — reported on repo settings → Imports, where "+ Add import" covered its hint. No absolute positioning was involved.

.hint-block carried margin: -0.35rem 0 0.9rem. That negative top margin exactly cancels the global h2 { margin: 0 0 0.35rem } — i.e. it was calibrated to a bare heading sitting above it. But .card-head is a flex row (align-items: center) sized by its tallest item, which is a 36px button.small, not the ~31px heading block. So the hint's negative margin ate into the button's box rather than the heading's, and the button's bottom overlapped the hint's first line.

The fix

Removes the enabling class rather than retuning the geometry.

1. CSS hardening (web/src/styles/) — fixes every page immediately, including the routes not migrated here:

.hint-block drops the negative top margin → margin: 0 0 0.9rem.
.card-head h2 resets to margin: 0; overflow-wrap: anywhere, matching .section-head h2. The row's height no longer depends on heading font metrics — headings load Source Serif 4 with font-display: swap, so an unreset heading margin made the row shift mid font-load.
.section-card-head states the gap under the head row explicitly (margin-bottom: var(--space-3), aligned with .section-head) instead of inheriting it by accident from the heading's own margin.

2. SectionCard (web/src/components/SectionCard.tsx) — owns the whole contract: `title`, an `action` slot for the head-row button, an optional `hint`, an optional `class`, and body `children`. All spacing between head, hint and body lives in this component and `.section-card-head` alone, so no call site can hand-roll it again. Body markup (error banners, add-forms, empty-vs-list, `<p class="empty">`) stays with the caller.

3. Migration — all 21 settings section cards now render through it:

`repo-settings/sections/`: Imports, Secrets, Schedules, Agents (×3), Autoland, Branches, Danger, General (×2), Integrations, Runner (×3)
`settings/sections/`: Notifications (×2), Agents (×3), General

One deviation from the issue's spec, on purpose

The issue asked for the explicit `margin-bottom` on `.card-head`. Applying it there would have regressed every list row: `.card-head` is also the head of an `<article class="card">` row in Tokens, History, Credentials, RepoCRs, Repos, RepoIssues, ProviderAuthCard and the push-device list, and each of those is immediately followed by `.card-sub` (`margin: 0.15rem 0 0`) that is meant to hug the title. 12px would have been inserted into all of them. The bottom margin therefore lives on a `.section-card-head` modifier that only `SectionCard` applies; the `h2` reset stays global on `.card-head` as specified. The acceptance criteria are unaffected — no negative margins remain in the head/hint area and the head-row height no longer depends on heading font metrics.

Scope discipline

Only a hint that was the head's immediate next sibling became the `hint` prop. Hints living deeper in a card body (conditional `` hints in Runner/Autoland/Agents, the trailing Incogni hint, the Notifications `` branches) are untouched where they are.
Per-item list-row `.card-head`s and the `div.card.form-card` sub-forms ("New import", "New secret", "New/Edit schedule") are untouched.
No copy changed anywhere.
Non-settings routes (Tokens, Credentials, History, Repos, RepoIssues, RepoCRs, …) keep their markup and are covered by the CSS hardening alone — they migrate in the follow-up umbrella #276.

Two settings test helpers located a card by `section.card > h2`; the title `h2` legitimately moved one level down into the head row, so both now use the descendant form. No assertion semantics changed.

Verification

```
npm run lint clean
npm run format:check All matched files use Prettier code style!
npm test 69 files, 996 tests passed
npm run build tsc --noEmit clean, vite build ok
```

`SectionCard.test.tsx` (6 cases) asserts the structural contract: title renders as the head-row `h2`; the action is the head row's last element child; the section's direct children are exactly head → hint → body in that order; both optional slots vanish when omitted; `class` appends to `card`.

Shipped bundle confirms the CSS actually landed:
```
.hint-block{color:var(--muted);margin:0 0 .9rem;font-size:.82rem;display:block}
.section-card-head{margin-bottom:var(--space-3)}
.card-head h2{overflow-wrap:anywhere;margin:0}
```

Still owed: the visual confirmation on repo settings → Imports (empty state), desktop and mobile widths — no browser was available in this session, as the issue anticipated.

Closes #275 ## The bug At empty states the head-row action button could print on top of the hint below it — reported on **repo settings → Imports**, where "+ Add import" covered its hint. No absolute positioning was involved. `.hint-block` carried `margin: -0.35rem 0 0.9rem`. That negative top margin exactly cancels the global `h2 { margin: 0 0 0.35rem }` — i.e. it was calibrated to a *bare heading* sitting above it. But `.card-head` is a flex row (`align-items: center`) sized by its tallest item, which is a 36px `button.small`, not the ~31px heading block. So the hint's negative margin ate into the button's box rather than the heading's, and the button's bottom overlapped the hint's first line. ## The fix Removes the enabling class rather than retuning the geometry. **1. CSS hardening** (`web/src/styles/`) — fixes every page immediately, including the routes not migrated here: - `.hint-block` drops the negative top margin → `margin: 0 0 0.9rem`. - `.card-head h2` resets to `margin: 0; overflow-wrap: anywhere`, matching `.section-head h2`. The row's height no longer depends on heading font metrics — headings load `Source Serif 4` with `font-display: swap`, so an unreset heading margin made the row shift mid font-load. - `.section-card-head` states the gap under the head row explicitly (`margin-bottom: var(--space-3)`, aligned with `.section-head`) instead of inheriting it by accident from the heading's own margin. **2. `SectionCard`** (`web/src/components/SectionCard.tsx`) — owns the whole contract: \`title\`, an \`action\` slot for the head-row button, an optional \`hint\`, an optional \`class\`, and body \`children\`. All spacing between head, hint and body lives in this component and \`.section-card-head\` alone, so no call site can hand-roll it again. Body markup (error banners, add-forms, empty-vs-list, \`<p class=\"empty\">\`) stays with the caller. **3. Migration** — all 21 settings section cards now render through it: - \`repo-settings/sections/\`: Imports, Secrets, Schedules, Agents (×3), Autoland, Branches, Danger, General (×2), Integrations, Runner (×3) - \`settings/sections/\`: Notifications (×2), Agents (×3), General ## One deviation from the issue's spec, on purpose The issue asked for the explicit \`margin-bottom\` on \`.card-head\`. Applying it there would have regressed every **list row**: \`.card-head\` is also the head of an \`<article class=\"card\">\` row in Tokens, History, Credentials, RepoCRs, Repos, RepoIssues, ProviderAuthCard and the push-device list, and each of those is immediately followed by \`.card-sub\` (\`margin: 0.15rem 0 0\`) that is meant to hug the title. 12px would have been inserted into all of them. The bottom margin therefore lives on a \`.section-card-head\` modifier that only \`SectionCard\` applies; the \`h2\` reset stays global on \`.card-head\` as specified. The acceptance criteria are unaffected — no negative margins remain in the head/hint area and the head-row height no longer depends on heading font metrics. ## Scope discipline - Only a hint that was the head's **immediate next sibling** became the \`hint\` prop. Hints living deeper in a card body (conditional \`<Show>\` hints in Runner/Autoland/Agents, the trailing Incogni hint, the Notifications \`<Switch>\` branches) are untouched where they are. - Per-item list-row \`.card-head\`s and the \`div.card.form-card\` sub-forms ("New import", "New secret", "New/Edit schedule") are untouched. - No copy changed anywhere. - Non-settings routes (Tokens, Credentials, History, Repos, RepoIssues, RepoCRs, …) keep their markup and are covered by the CSS hardening alone — they migrate in the follow-up umbrella #276. Two settings test helpers located a card by \`section.card > h2\`; the title \`h2\` legitimately moved one level down into the head row, so both now use the descendant form. No assertion semantics changed. ## Verification \`\`\` npm run lint clean npm run format:check All matched files use Prettier code style! npm test 69 files, 996 tests passed npm run build tsc --noEmit clean, vite build ok \`\`\` \`SectionCard.test.tsx\` (6 cases) asserts the structural contract: title renders as the head-row \`h2\`; the action is the head row's last element child; the section's direct children are exactly head → hint → body in that order; both optional slots vanish when omitted; \`class\` appends to \`card\`. Shipped bundle confirms the CSS actually landed: \`\`\` .hint-block{color:var(--muted);margin:0 0 .9rem;font-size:.82rem;display:block} .section-card-head{margin-bottom:var(--space-3)} .card-head h2{overflow-wrap:anywhere;margin:0} \`\`\` **Still owed:** the visual confirmation on repo settings → Imports (empty state), desktop and mobile widths — no browser was available in this session, as the issue anticipated.

dominik.polakovics added 1 commit

2026-08-04 14:40:11 +02:00

fix(web): section cards own their head/hint spacing, extract SectionCard

ci / native (pull_request) Successful in 7m46s

Details

ece526b034

At empty states a section card's head-row action button could print on top
of the hint below it — reported on repo settings → Imports, where "+ Add
import" covered its hint. No absolute positioning was involved. `.hint-block`
pulled itself up with a negative top margin calibrated to cancel a bare
`h2`'s own bottom margin, but a `.card-head` flex row is sized by its tallest
item — a 36px `button.small`, not the ~31px heading block. The negative
margin therefore ate into the button's box instead of the heading's.

Rather than retune the geometry, remove the class of bug:

- `.hint-block` drops the negative top margin (`margin: 0 0 0.9rem`).
- `.card-head h2` resets to `margin: 0; overflow-wrap: anywhere`, matching
  `.section-head h2`. The row's height no longer depends on heading font
  metrics — headings load Source Serif 4 with `font-display: swap`, so an
  unreset heading margin made the row shift mid font-load.
- `.section-card-head` states the gap under the head row explicitly instead
  of inheriting it by accident from the heading. Deliberately a separate
  class from `.card-head`: that class is also a *list row*'s head (Tokens,
  History, Credentials, RepoCRs, push devices), where it must keep hugging
  its `.card-sub`.
- New `SectionCard` (title, action slot, hint, children) owns the whole
  contract so no call site can hand-roll it again. All 21 settings section
  cards under repo-settings/sections/ and settings/sections/ render through
  it; body markup (error banners, add-forms, empty-vs-list) stays with the
  caller.

Only a hint that was the head's immediate next sibling became the `hint`
prop — hints living deeper in a card body are untouched. Per-item list-row
`.card-head`s are untouched. Non-settings routes keep their markup and are
covered by the CSS hardening alone; they migrate in #276.

The two settings test helpers that located a card by `section.card > h2`
now use the descendant form, since the title `h2` moved one level down into
the head row.

dominik.polakovics commented

2026-08-04 14:50:38 +02:00

Author

Owner

[autoland] verdict: pass

Verdict: PASS with CONCERNS — validated, awaiting confirmation. Not auto-merged.

Signal relied on

labctl pr checks 277 → aggregate success; single check ci / native green in 7m46s (run 382). That job is unconditional (on: pull_request, no path gate) and runs the full SPA gate this diff touches — npm run lint, npm run format:check, npm test, npm run build — plus the ui-tagged Go build/test and golangci-lint. Full reach over the changed paths; nothing re-run locally.

Conventions

Title fix(web): … — Conventional Commits ✓
Closes #275 present ✓
Head is one commit (ece526b) on top of the base tip c8bb2e0 — no conflict, no resolution commit ✓
Diff scope matches #275: CSS hardening + SectionCard + the 21 settings-section migrations + two test-helper selector updates. No drive-by changes ✓

Review notes (all clean)

web/src/styles/cards.css:51 — .card-head h2 { margin: 0; overflow-wrap: anywhere } correctly mirrors .section-head h2 (web/src/styles/shell.css:392).
web/src/styles/banners.css:61 — .danger-zone h2 is a descendant selector, so it still matches now that the h2 moved into the head row; it only sets color, so it does not collide with the new margin reset.
No section.card > h2 direct-child selectors remain anywhere in web/src — both call sites were updated (routes/settings/harness.tsx:262, routes/settings/sections/Notifications.test.tsx:221), and cardByHeading delegates to cardByHeadingOrNull, so the single change covers both.
.spacer { flex: 1 } (web/src/styles/layout.css:9) is global and inert when empty, so SectionCard's always-rendered spacer is safe with no action.
The documented deviation — bottom margin on a .section-card-head modifier (web/src/styles/cards.css:64) rather than on .card-head as #275 literally specified — is correct, not a shortcut: .card-head is also the head of list-row cards followed by .card-sub (margin: 0.15rem 0 0), which are meant to hug their title. The stated acceptance criteria are still met.

Concerns (non-blocking)

The issue's pre-merge visual gate is unmet. #275 states it twice — "The reporter verifies visually before merge" and, in the acceptance criteria, "Reporter visually confirms on repo settings → Imports (empty state) … desktop and mobile widths." The PR body flags this as Still owed (no browser in that session). Nothing in CI substitutes for it: SectionCard.test.tsx asserts the structural contract (head → hint → body ordering, slot presence/absence), not that the button no longer overlaps the hint. Since this is the gate the issue owner placed before merge, an autoland merge would skip it — hence approve-only.
The CSS hardening reaches every route, and the unmigrated ones have no coverage at all. .hint-block's margin-top going -0.35rem → 0 (web/src/styles/forms.css:56) and the global .card-head h2 { margin: 0 } shift spacing on ~23 non-test hint-block call sites and on every list-row card head — Tokens, History, Credentials, Repos, RepoIssues, RepoCRs, ProviderAuthCard, the push-device list. That breadth is the intent of #275 ("fixes every page immediately"), and each shift is small and in the safe direction (more room, not less), but it is unverified visually. Worth a glance at one list-heavy route (History or Tokens) alongside the Imports check in concern 1.

Neither concern is marked blocking. Once the visual confirmation lands, this is a clean merge.

[autoland] verdict: pass **Verdict: PASS with CONCERNS** — validated, awaiting confirmation. Not auto-merged. ## Signal relied on `labctl pr checks 277` → aggregate **success**; single check `ci / native` green in 7m46s (run 382). That job is unconditional (`on: pull_request`, no path gate) and runs the full SPA gate this diff touches — `npm run lint`, `npm run format:check`, `npm test`, `npm run build` — plus the `ui`-tagged Go build/test and golangci-lint. Full reach over the changed paths; nothing re-run locally. ## Conventions - Title `fix(web): …` — Conventional Commits ✓ - `Closes #275` present ✓ - Head is one commit (`ece526b`) on top of the base tip `c8bb2e0` — no conflict, no resolution commit ✓ - Diff scope matches #275: CSS hardening + `SectionCard` + the 21 settings-section migrations + two test-helper selector updates. No drive-by changes ✓ ## Review notes (all clean) - `web/src/styles/cards.css:51` — `.card-head h2 { margin: 0; overflow-wrap: anywhere }` correctly mirrors `.section-head h2` (`web/src/styles/shell.css:392`). - `web/src/styles/banners.css:61` — `.danger-zone h2` is a descendant selector, so it still matches now that the `h2` moved into the head row; it only sets `color`, so it does not collide with the new margin reset. - No `section.card > h2` direct-child selectors remain anywhere in `web/src` — both call sites were updated (`routes/settings/harness.tsx:262`, `routes/settings/sections/Notifications.test.tsx:221`), and `cardByHeading` delegates to `cardByHeadingOrNull`, so the single change covers both. - `.spacer { flex: 1 }` (`web/src/styles/layout.css:9`) is global and inert when empty, so `SectionCard`'s always-rendered spacer is safe with no action. - The documented deviation — bottom margin on a `.section-card-head` modifier (`web/src/styles/cards.css:64`) rather than on `.card-head` as #275 literally specified — is correct, not a shortcut: `.card-head` is also the head of list-row cards followed by `.card-sub` (`margin: 0.15rem 0 0`), which are meant to hug their title. The stated acceptance criteria are still met. ## Concerns (non-blocking) 1. **The issue's pre-merge visual gate is unmet.** #275 states it twice — "The reporter verifies visually before merge" and, in the acceptance criteria, "Reporter visually confirms on repo settings → Imports (empty state) … desktop and mobile widths." The PR body flags this as **Still owed** (no browser in that session). Nothing in CI substitutes for it: `SectionCard.test.tsx` asserts the structural contract (head → hint → body ordering, slot presence/absence), not that the button no longer overlaps the hint. Since this is the gate the issue owner placed *before* merge, an autoland merge would skip it — hence approve-only. 2. **The CSS hardening reaches every route, and the unmigrated ones have no coverage at all.** `.hint-block`'s `margin-top` going `-0.35rem → 0` (`web/src/styles/forms.css:56`) and the global `.card-head h2 { margin: 0 }` shift spacing on ~23 non-test `hint-block` call sites and on every list-row card head — Tokens, History, Credentials, Repos, RepoIssues, RepoCRs, ProviderAuthCard, the push-device list. That breadth is the intent of #275 ("fixes every page immediately"), and each shift is small and in the safe direction (more room, not less), but it is unverified visually. Worth a glance at one list-heavy route (History or Tokens) alongside the Imports check in concern 1. Neither concern is marked blocking. Once the visual confirmation lands, this is a clean merge.

dominik.polakovics merged commit d2b61661ee into main

2026-08-04 15:55:48 +02:00

dominik.polakovics deleted branch afk/275

2026-08-04 15:55:49 +02:00

dominik.polakovics referenced this pull request from a commit

2026-08-04 15:55:50 +02:00

Merge pull request 'fix(web): section cards own their head/hint spacing, extract SectionCard' (#277) from afk/275 into main