feat(desktop): the draft's palette, and the primitives every pane is built from - #30
Merged
Conversation
…e is built from Group 2 of plans/desktop-ui.md. 8.1 shipped tokens only, and shipped them from a palette of its own invention while `design/desktop-draft.html` sat in the repository being nothing's acceptance criterion. This is the other half, and it takes the draft's answer rather than the one 8.1 guessed. - `tokens.css` becomes `globals.css`, with the draft's names and values: the cold slate instrument, the warm paper it holds, amber `--primary` meaning provenance, the semantic set kept separate from the accent, the 10→32px scale, `--row: 30px` and `--control: 28px`. Every `--surface-*`/`--ink-*` reference migrates in the same commit, because a half-migrated palette is two palettes — and the type scale in particular shifts under its own names (`--text-xs` was 12px and is now 11px), so a partial migration would have been silently wrong rather than obviously unfinished. - `lucide-react` and `clsx`, in the manifest and in `docs/stack.md` with the line each earned. No shadcn/ui runtime: what the draft means by "token names match shadcn/ui" is the variable names, and shadcn is copied-in source rather than a package. - The primitives, one file each under `src/renderer/ui/`: Button, IconButton, Pill, Card, Table, Switch, Dialog, Sheet, Drawer, SearchInput. Each exports the function that decides its classes, so the decision is testable and the component stays arrangement — the convention the renderer already had. - `Ask.tsx` from group 1 now sits on the `Dialog` primitive, so the focus trap, Escape and the return of focus are settled in one place rather than copied into three overlays. - Focus is visible, from one `:focus-visible` rule, and a test reads the stylesheet to keep it that way: exactly one `outline: none` is allowed, it is the search field's, and the ring it removes is put back on the box. Verified: `pnpm test:coverage` (the whole workspace, 76% floor), `pnpm lint`, `pnpm run typecheck`, `pnpm --filter @open-wiki/desktop run build:renderer`, `npx @protonspy/scc validate` — all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC
…con size Both findings from the code review, and the first one is the more instructive: the commit before this claimed the type-scale shift had been handled and it had been handled only where the draft draws the class. Everything the draft does not draw — the chrome, the frontmatter, the error and empty lines, the editor, the findings, the launcher's labels, the doorway, the question box's own field label — kept its variable name and so lost a pixel each. That is exactly the silent half-migration the message said had been avoided. - Each of those is shifted a tier, to the size it had: old `--text-xs` (12px) becomes `--text-sm`, old `--text-sm` (13px) becomes `--text-base`. `.chrome` loses its declaration entirely, because the draft's titlebar sets none and inherits the body's 13px. - Inline code in prose takes the draft's own `.inline-code` rule — `0.92em`, relative — rather than a fixed step, so it tracks whatever it sits inside. - `.icon-btn` is 24px, which is what the draft draws, and the `--sm` variant invented here is gone. A size the draft does not draw is a decision nobody made that groups 3 to 8 would all have inherited. - Two tests for the class of bug this was. CSS is silent about `var(--gone)`: the property falls back to what it inherits, nothing errors, and the rule quietly stops applying. One asserts every referenced custom property is defined; the other that no rule still reaches for the 8.1 palette. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC
…ty is one The review was right and the correction is small but the principle behind it is not: the previous commit restored every shifted selector to the pixel size the *old application* had chosen, and `.empty` is the one of them the draft draws itself — at `--text-sm`. Restoring it to 13px moved a rule that was already right, which is precisely the failure the round before it was about. `.page code` and `.doorway code` keep `0.92em`, and now say why in the file: the draft sizes inline code in prose relatively, so a code span tracks the paragraph it interrupts rather than being one fixed size in a reader set larger than the chrome around it. That is a deliberate departure from "restore the pixel", not an oversight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
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
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.
Group 2 of
plans/desktop-ui.md— tasks 2.1 to 2.4, ticked.8.1shipped tokens only — and shipped them from a palette of its own invention, whiledesign/desktop-draft.htmlsat in the repository being nothing's acceptance criterion. This is the other half, and it takes the draft's answer rather than the one 8.1 guessed.What changed
2.1 — the tokens.
tokens.cssbecomesglobals.csswith the draft's names and values: the cold slate instrument, the warm paper it holds, amber--primarymeaning provenance, the semantic set kept separate from the accent, the 10→32px scale,--row: 30pxand--control: 28px. The names are shadcn/ui's on purpose — that is what the draft means by "the port is a copy intoglobals.css".Every
--surface-*/--ink-*reference migrates here, because a half-migrated palette is two palettes. The type scale is the part that bites:--text-xswas 12px and is now 11px,--text-smwas 13px and is now 12px, so a reference that looks untouched has silently moved. It took two review rounds to get right, and the rule that settled it is the draft wins where the draft has an opinion, and the old pixel wins where it does not.2.2 — two dependencies, in the manifest and in
docs/stack.mdwith the line each earned. No shadcn/ui runtime: shadcn is copied-in source, and installing a package to get a naming convention would be adopting a dependency for nothing.2.3 — the primitives, one file each under
src/renderer/ui/: Button, IconButton, Pill, Card, Table, Switch, Dialog, Sheet, Drawer, SearchInput. Each exports the function that decides its classes, so the decision is testable and the component stays arrangement — the convention the renderer already had.Ask.tsxfrom group 1 now sits on theDialogprimitive, so the focus trap, Escape and the return of focus are settled in one place instead of copied into three overlays.2.4 — focus. One
:focus-visiblerule for the whole document, so a primitive cannot forget it, and a test that reads the shipped stylesheet: exactly oneoutline: noneis allowed, it is the search field's, and the ring it removes is put back on the box around it.How it was verified
pnpm test:coverage(whole workspace, 76% floor, exit 0) ·pnpm lint·pnpm run typecheck·pnpm --filter @open-wiki/desktop run build:renderer·npx @protonspy/scc validate— no findings.code-reviewandsecurity-reviewon the diff, thencode-reviewagain on each round of fixes. Three findings, all closed:var(--gone)— the property falls back to what it inherits and the rule quietly stops applying..icon-btnwas--control(28px) where the draft draws 24px, and an--smvariant was invented that the draft never draws. A size the draft does not draw is a decision nobody made that groups 3 to 8 would all have inherited..emptyto 13px, when the draft draws.emptyitself at 12px. That is the same failure in the other direction, and it is what produced the rule stated above.The third round was a one-line revert of a value the reviewer identified precisely and I checked against the draft myself, so it was not sent back for a fourth review.
Two things left open on purpose
--warnis gone. The draft has no warning colour, so.finding--warningnow shares amber with citations. The security review flagged it as a low legibility note rather than a state collision — findings always carry their code, message and fix as text — but it is a real narrowing of the palette and worth a look.🤖 Generated with Claude Code
https://claude.ai/code/session_01HBt5LRVJd5Z7nagJGRwvSC