From 2f060e9baf8e4e8051371265ec9f6ae491784580 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Wed, 4 Nov 2026 15:29:07 +0100 Subject: [PATCH] :sparkles: Add skills for the DS migration epic (#12739) * :sparkles: Add skills for the DS migration epic Two skills that drive the DS migration (#12375) one area at a time, so anyone working with an agent follows the same flow. refactor-scss moves one .scss off frontend/resources/styles/common/ refactor/ to the DS tokens, mixins and px2rem as a pure refactor, with a self-review for the issues reviewers flag and a zero-tolerance visual check. migrate-ds-area takes one area of a group issue (buttons and icons, selects, ...): claim it in the group's PRs table, add a visual test, change the controls, run refactor-scss, compare before and after with the bundled diff scripts, then open the draft PR and post the QA comment. It also lists devenv traps seen while migrating the first areas. AI-assisted-by: claude-opus-5-5 * :sparkles: Add the claim rules to the migrate-ds-area skill The group issue body is saved whole, so two people claiming at the same time can lose a row. The skill now rereads the body after saving and puts the row back if it is gone. A claim with no PR for 7 days is free once its owner has been asked in a comment and did not answer. AI-assisted-by: claude-opus-5-5 --- .agents/skills/migrate-ds-area/SKILL.md | 330 ++++++++++++++ .../skills/migrate-ds-area/scripts/diff.py | 25 ++ .../skills/migrate-ds-area/scripts/trip.py | 33 ++ .agents/skills/refactor-scss/SKILL.md | 421 ++++++++++++++++++ 4 files changed, 809 insertions(+) create mode 100644 .agents/skills/migrate-ds-area/SKILL.md create mode 100644 .agents/skills/migrate-ds-area/scripts/diff.py create mode 100644 .agents/skills/migrate-ds-area/scripts/trip.py create mode 100644 .agents/skills/refactor-scss/SKILL.md diff --git a/.agents/skills/migrate-ds-area/SKILL.md b/.agents/skills/migrate-ds-area/SKILL.md new file mode 100644 index 0000000000..d06d40a4f5 --- /dev/null +++ b/.agents/skills/migrate-ds-area/SKILL.md @@ -0,0 +1,330 @@ +--- +name: migrate-ds-area +description: Migrate one Penpot UI area to the Design System components for one group of the DS migration epic (#12375): buttons and icons (icon-button*, button*, icon*), selects (select*) and later groups. Moves the area's SCSS off the deprecated styles, proves every change with a zero-tolerance visual test, then opens the draft PR and updates the group issue (PRs table row, QA comment). Use when the user asks to migrate an area or module to the DS, continue a DS migration group, or names an area of a group issue. +--- + +# Skill: migrate-ds-area + +Move one UI area (one `.cljs` + `.scss` pair, or a few related modules) +from legacy components and raw HTML to the Design System components, +prove with pixels what changed, and ship it as a draft PR that QA can +test. + +The epic is #12375. Each control type is a group with its own issue +(buttons and icons #12376, selects #12377, radios #12378, …). The group +issue holds the plan (order, waiting areas), the changes QA should +expect, and the PRs table where people claim areas. Read it first. + +## Input + +- The group issue (e.g. `#12376`) and the area: a module path (e.g. + `dashboard/fonts`) or an area named in the group issue. +- No area given: take the next unclaimed area in the group issue's plan + order, and say which one. + +## Ground rules + +- Never `git push`. The user pushes; then you open the PR. +- PRs go to `develop`; branches start from `origin/develop`. +- **Stay in the group.** Only the controls of this group change. In the + buttons/icons group, selects, radios, numeric/text inputs, checkboxes, + menus (`%menu-*`, `%dropdown-*`) and modal structure (`%modal-*`) stay; + a checkbox tick or an input label icon stays legacy. +- **Accept the DS look.** Do not add CSS to imitate legacy sizes or + colors. Do keep positions: icons and labels must not move unless the + DS size forces it, and then say so. +- No negative margins to compensate; use positive DS spacing or + `px2rem()`. +- **Gaps in the DS wait.** An area that needs an icon or a feature the + DS lacks keeps its legacy control; say so in the PR. +- Skip legacy shared components that another group will replace + (`context_menu_a11y`, `forms`, `select`, `dropdown`, `radio_buttons`, + `search_bar`, `title_bar`, …): they wait for that group's cleanup, + where they are updated if still used or deleted if not. +- Debug-only UI gets no QA comment and no compare image. + +## Devenv + +Run every test, lint and `pnpm` command inside the workspace container, +never on the host (a host `pnpm` relinks `node_modules` and breaks the +container's watcher). The container is `penpot-devenv-ws-main`, where +`` is the workspace number (`ws0` is the main repo; `wsN` lives in +`~/.penpot/penpot_workspaces/wsN`). Write output to a file, then read it: + +```bash +C=penpot-devenv-ws1-main # your workspace +docker exec -u penpot -w /home/penpot/penpot/frontend $C \ + bash -lc '' > /out.txt 2>&1 +``` + +When the app does not behave as the code says, check the build first: + +- **Stale build after a branch switch** ("is not a function", or a page + with empty `stl/css` classes): `touch` the stale `.cljs`/`.scss`, wait + for the compiled file under `frontend/resources/public/` to update, + hard-reload. +- **A whole area unstyled:** a new `.scss` added after the watcher + started is missing from `main.css` (grep `main.css` for the module + prefix), or `main.css` was written corrupted after many `.scss` + changed at once. Touch a `.scss`; if the module is still missing, + restart the watcher in the workspace's tmux session (`penpot:0`, + `./scripts/watch app`). +- **Blank app after switching between `develop` and `staging`:** + dependencies differ. Run `pnpm install --frozen-lockfile` in the + container and restart the watcher. + +## Workflow + +### 1. Claim the area and branch + +1. Read the group issue: plan, waiting areas, expected changes, PRs table. +2. Add a row to the PRs table with no PR number, the user's @login as + Owner and the state `Claimed` (`gh issue view/edit --body-file`). + Read the body right before editing, and read it again after saving: + when two people edit at the same time, GitHub keeps only the last + edit, so a row can vanish. Put yours back if it is gone. + Skip an area someone else already claimed, unless the claim has had + no PR for 7 days and its owner did not answer a comment asking. +3. `git switch -c -ds-- origin/develop`. +4. Survey the area: every control of the group in the `.cljs`, the + legacy icons, the placeholders and mixins the `.scss` uses, the + `div`/`span` with `:on-click`. + +### 2. Visual baseline + +Add a `test.describe(" controls", …)` block to the matching spec +in `frontend/playwright/ui/visual-specs/` (project `ds`, dark theme). +Put it next to the blocks of the same screen, not always at the end of +the file: two PRs that both append at the end conflict. + +- Every shot: `toHaveScreenshot({ maxDiffPixelRatio: 0, threshold: 0 })`. +- Show every changed control at rest, on hover and open or pressed. + Capture the element or the smallest stable container. +- Locators must work before and after the change: text, role, testid, + `[class*='…']`, or `a.or(b)` when the element type changes (raw + `[class*='menu-btn']` before, `getByRole("button", { name: "Options" })` + after). Scope `.or()` to the area so it cannot match elsewhere. +- Missing data? Add a fixture under `frontend/playwright/data/` and mock + it (`dashboardPage.mockRPC(...)`, `DashboardPage.mockConfigFlags` for + flags). Watch for expired or inconsistent fixtures (dates in the past, + wrong file for a mock). +- Capture with `--update-snapshots=all`: + ```bash + npx playwright test --project=ds -g "" --update-snapshots=all + ``` + Plain `--update-snapshots` keeps a snapshot within the project's 0.5% + tolerance, so a real diff looks like 0. +- Copy the snapshots (`.spec.js-snapshots/-*`) to + `/diffs//before/` and look at them: the state you + wanted must really be on screen. + +### 3. Controls commit + +#### Buttons and icons + +| Legacy | DS | +|---|---| +| icon-only raw button | `[:> icon-button* {:variant "ghost" :aria-label (tr …) :icon i/x :on-click f}]` (always pass `:variant`; `"action"` for 24px icon toggles) | +| pressed/selected | `:aria-pressed (boolean x)` | +| text button | `[:> button* {:variant "primary"/"secondary"/"ghost"/"destructive" :type "button"} label]` | +| `[:input {:type "button"}]` | `button*` | +| modal close | `icon-button*` ghost, `:aria-label (tr "labels.close")`, `:tooltip-class` carrying the corner position | +| legacy icon outside a button | `[:> icon* {:icon-id i/x :class …}]` | + +- Not every raw button changes: rows, tabs, swatches, avatar widgets, + full-height segments, round custom buttons and link-style text buttons + stay raw (their legacy icon still changes). List the kept ones in the + PR. +- A `div`/`span` with `:on-click` in the area becomes a `[:button]` + (with `aria-expanded`/`aria-haspopup` when it opens something). An + `[:a]` without `href` stays; list it in the PR. +- **Tooltip wrapper.** `icon-button*` renders inside a tooltip trigger + wrapper. Position (`position: absolute`, grid placement, margins + against siblings) goes on `:tooltip-class`; look (`--button-height`, + colors) goes on `:class`. +- **Tooltip at the right edge.** Small icon buttons at the right end of + a sidebar row or title bar get `:tooltip-placement "top-left"`; list + it as an expected change. +- **Sizes through custom properties:** `--button-height: #{px2rem(64)}` / + `--button-width`. Sass does not evaluate functions inside custom + properties without `#{}`. +- **A grown `button*` keeps its label at the start**: add + `justify-content: center` when it is wider than its content. +- **Icon colors:** DS icons use `stroke: currentColor`. Set `color` on + the icon class, not `stroke`/`fill`. A legacy `color: transparent` left + on the button or wrapper makes the DS icon invisible: drop it. A class + that redefines the color variable on the icon breaks the hover that + changes it on the button: define the variable on the button. +- **Every DS icon needs a color of its own** (on its class or a parent): + one that inherits none shows in the devenv debug yellow. +- **Icon sizes:** `"s"` draws 12px inside a 16px box (compensate the + margin by 2px per side to keep the drawing in place); `"m"` 16px; `"l"` + 24px; `"xl"` 32px. +- **Stroke width:** DS icons keep the DS stroke width. Never set + `stroke-width` on a DS icon to match the legacy one; a thinner or + thicker line is part of the DS look. +- **Legacy placeholders that style `svg` descendants** (e.g. + `%dropdown-element-base` → `span svg { 12px box; stroke }`) override a + DS icon placed inside them: give the icon class its own box + (`inline-size`/`block-size` 16px) and `stroke: currentcolor` with more + specificity. +- **Flex shrink:** a DS svg inside a small flex button can shrink and be + cut; add `flex-shrink: 0` to the icon class. +- **Rotated legacy arrows:** prefer `i/arrow-left`/`-right`/`-up`/`-down` + over rotating `i/arrow` (same path), unless a rotation class drives the + open/closed state. +- **Menus anchored by the button size** move when the button grows; open + them from the top (`inset-block-start: calc(100% + var(--sp-xs))`) or + fix the context menu offset, and say so. + +#### Selects + +`[:& select {:default-value v :options [{:value :label}] :on-change f}]` +→ `[:> select* {:default-selected :options :on-change f}]` +(`app.main.ui.ds.controls.select`). + +- **Ids are non-blank strings.** A blank id shows a dimmed "--", and an + id that matches no option falls back to the first one. Give mixed, + none and auto their own ids (`"multiple"`, `"none"`, `"auto"`) and map + them back in the handler. Use `""` only where "--" is the wanted look. +- **Convert the value.** The legacy select ran `d/read-string`, so + handlers got keywords, numbers or uuids; `select*` passes the `:id` + string. Convert with `keyword`, `d/parse-integer` or `uuid/parse`. +- **Memoized options, live selection.** `select*` resets its selection + when `:default-selected` or the `:options` identity changes. Build + options with `mf/with-memo` and `mapv` (the schema wants a non-empty + vector) and pass `:default-selected` from the live value, never a + constant. +- **Never pass nil.** In dev builds a present prop with value nil + (`:class`, `:wrapper-class`, `:disabled`, `:default-selected`) throws + `invalid props on component select*`. Use `(boolean x)`, `d/nilv`, or + add the prop only when it has a value. +- **Props the legacy select ignored stay out**: `select*` passes rest + props to its button, so they would reach the DOM. +- **Classes:** `:wrapper-class` for layout, `:class` for look. +- **Portal:** add `:has-portal true` only where the visual test shows the + list cut at the bottom of a panel, and say so in the QA comment. +- **Locators:** the button is a `combobox` with no accessible name; use + `getByRole("combobox").filter({ hasText: "Linear" })` (it matches the + legacy select too) and `getByRole("option", { name: … })`. +- **Keyboard:** the arrows no longer change the value while the list is + closed. It is an expected change, already listed in #12377. + +#### All groups + +- New labels: add the key to `en.po` and `es.po` by hand, in sorted + position, with a `#:` reference to the cljs line. Never run the + translations sync. +- The `.scss` only gets what the new controls need in this commit. +- Some e2e specs are not Prettier-clean on develop: never run + `prettier --write` on a whole spec; format only your block. + +Then wait for the watcher (grep the compiled JS/CSS under +`frontend/resources/public/` for the change), recapture with `=all`, +copy to `after/`, diff: + +```bash +SK=.agents/skills/migrate-ds-area/scripts +python3 -I $SK/diff.py # per-file pixel count + bbox +python3 -I $SK/trip.py # before | after | red diff +``` + +`trip.py` pads images of different sizes, so a taller "after" still +renders. +Explain every diff. Fix what is not a DS-look change (moved icons, cut +icons, lost hover color). Rerun a shot that changes on unchanged code +before trusting it; flaky spots seen so far: the invite modal textarea +border, the search input caret, the user account page, the path handler +menu trigger. + +Lint, then commit (read `mem:workflow/creating-commits` first): + +```bash +npx prettier --check ; npx stylelint ; clj-kondo --lint ; cljfmt check +git add -f # -f: visual-specs/ is gitignored +./scripts/check-commit +``` + +A plain `git add` with an ignored path in the list adds nothing; always +`-f`, and check `git show --stat`. Never commit the screenshots. +Subject: `:recycle: Use DS in `. A pre-existing bug +found and fixed on the way goes in its own `:bug:` commit. + +### 4. SCSS commit + +Run the `refactor-scss` skill on every `.scss` the PR touched (only +those), in its own commit. Keep `refactor/` only for component-family +placeholders (`%modal-*`, `%menu-*`, `%input-*`, `%checkbox-*`) and say +why in a comment above the `@use`. Unused selectors are deleted, not +commented out (list them in the commit body). Typography without an +exact match uses the closest DS typography. Run the skill's +**Self-review** before committing: no bare element selectors (add +classes in the `.cljs`), no copied properties that do nothing, z-index +tokens by purpose. Compare against the controls commit: it should be +0 px; explain anything else (typically a typography or border change). +For files that several modules share or that are huge, stop and propose +a separate PR instead. + +### 5. Hand off, PR and tracking + +1. Report to the user: commits, visible changes, kept raw controls, + pre-existing bugs, and the compare image path + (`/diffs//compare.png`, before | after | diff). Give + the push command: `git push -u origin ` (the branch tracks + `origin/develop`, so a bare `git push` is refused). +2. After the user pushes, check `git rev-parse HEAD origin/`, + then (read `mem:workflow/creating-prs` first): + ```bash + gh pr create --repo penpot/penpot --draft --base develop --head \ + --title "" --body-file \ + --label "WG adoption" --label "no issue required" \ + --milestone --project Main + gh pr edit --repo penpot/penpot --remove-label "needs triage" + ``` + Never add `no changelog` to a PR: that label goes on issues. + Body: AI note, `Part of # (DS migration epic #12375).`, a + short What / How, the kept raw controls, an empty `## Screenshots` + when something is visible (the user attaches the image). Never + `Closes #`. Keep it short: reviewers find long PRs hard to read. +3. In the group issue's PRs table, fill your row: + `| # | : () | @ | Draft |`. +4. Post a QA comment on the group issue: `**#: **`, *Where* (how + to reach it), *Expected changes* (every visible change; QA treats + anything not listed as a bug), *No other change* (what still works + the same). + +Do not mark the draft ready or ask for review: the user decides when. + +### 6. Review fixes + +When the reviewer comments on a PR: + +1. Fix each comment in a new commit on the branch (never amend pushed + commits). A comment you do not apply gets a short reason the user can + post, with an image when it is visual. +2. Rerun the area's visual spec before and after the fix with `=all`. + Build a compare image for every visible change and give the user its + full path, saying which PR it goes to. +3. Update everything the fix makes stale, in the same turn: the QA + comment (*Expected changes* must list the new visible changes), the + PRs table row, and the PR body. +4. When working on several PRs in one go, report per PR: branch, new + commit, what changed, image path. Never mix two branches in one list. +5. Give the push command per branch. If a rebase is coming soon, say + that pushing now costs one extra CI run; the user decides. +6. After a round of review fixes, check whether the same issue exists in + the user's other open PRs of the group, and fix it there too. + +### 7. After merges + +Whenever PRs merge (or go from draft to ready), sync the state column of +the group's PRs table with GitHub for **every** row, not only your own: +read each row's PR (`gh pr view --json state,isDraft`) and set +Merged / In review / Draft. The table drifts when other people merge. + +## Output + +- Branch with 2 commits (controls, SCSS) plus any `:bug:` commit. +- Visual spec block with zero-tolerance shots. +- Compare image in the scratch folder. +- After the push: draft PR, row and QA comment in the group issue. diff --git a/.agents/skills/migrate-ds-area/scripts/diff.py b/.agents/skills/migrate-ds-area/scripts/diff.py new file mode 100644 index 0000000000..9792f37d8a --- /dev/null +++ b/.agents/skills/migrate-ds-area/scripts/diff.py @@ -0,0 +1,25 @@ +"""Pixel diff of two folders of PNG snapshots with the same file names. + +Usage: python3 -I diff.py +Prints, per file, the number of differing pixels and their bounding box, +or the two sizes when they differ. +""" +import os +import sys + +from PIL import Image, ImageChops + +before, after = sys.argv[1], sys.argv[2] +for name in sorted(os.listdir(before)): + other = os.path.join(after, name) + if not os.path.exists(other): + print(name, "missing in", after) + continue + a = Image.open(os.path.join(before, name)).convert("RGB") + b = Image.open(other).convert("RGB") + if a.size != b.size: + print(name, "size", a.size, b.size) + continue + diff = ImageChops.difference(a, b) + count = sum(1 for p in diff.getdata() if p != (0, 0, 0)) + print(name, count, diff.getbbox()) diff --git a/.agents/skills/migrate-ds-area/scripts/trip.py b/.agents/skills/migrate-ds-area/scripts/trip.py new file mode 100644 index 0000000000..dde5c96bf0 --- /dev/null +++ b/.agents/skills/migrate-ds-area/scripts/trip.py @@ -0,0 +1,33 @@ +"""Before | after | red diff image, one row per snapshot. + +Usage: python3 -I trip.py +Both folders hold PNGs with the same names (crop them first if needed). +""" +import os +import sys + +from PIL import Image, ImageChops + +before, after, out, scale = sys.argv[1], sys.argv[2], sys.argv[3], int(sys.argv[4]) +rows = [] +for name in sorted(os.listdir(before)): + a = Image.open(os.path.join(before, name)).convert("RGB") + b = Image.open(os.path.join(after, name)).convert("RGB") + w, h = max(a.width, b.width), max(a.height, b.height) + pad_a = Image.new("RGB", (w, h), (200, 200, 200)); pad_a.paste(a, (0, 0)) + pad_b = Image.new("RGB", (w, h), (200, 200, 200)); pad_b.paste(b, (0, 0)) + mask = ImageChops.difference(pad_a, pad_b).convert("L").point(lambda v: 255 if v else 0) + red = Image.new("RGB", (w, h), (255, 255, 255)) + red.paste((255, 0, 0), mask=mask) + row = Image.new("RGB", (w * 3 + 20, h), (200, 200, 200)) + for i, im in enumerate([pad_a, pad_b, red]): + row.paste(im, (i * (w + 10), 0)) + rows.append(row.resize((row.width * scale, row.height * scale), Image.NEAREST)) +width = max(r.width for r in rows) +height = sum(r.height + 10 for r in rows) +canvas = Image.new("RGB", (width, height), (255, 255, 255)) +y = 0 +for r in rows: + canvas.paste(r, (0, y)) + y += r.height + 10 +canvas.save(out) diff --git a/.agents/skills/refactor-scss/SKILL.md b/.agents/skills/refactor-scss/SKILL.md new file mode 100644 index 0000000000..019dface29 --- /dev/null +++ b/.agents/skills/refactor-scss/SKILL.md @@ -0,0 +1,421 @@ +--- +name: refactor-scss +description: Migrate one Penpot SCSS module off the deprecated styles under frontend/resources/styles/common/refactor/ to the current Design System (ds/ tokens, mixins and px2rem). Pure refactor, no visual change. Use when the user asks to refactor, migrate or clean up a .scss file to the DS. +--- + +# Skill: refactor-scss + +Refactor a single SCSS file so that it no longer depends on the deprecated +styling system, using the current Design System instead. + +This is a **pure refactor**. Do NOT introduce visual or behavioral changes. +Preserve the generated CSS behavior exactly unless a rule below requires +otherwise. The rules that allow a small visible change (closest DS +typography, DS border width) want it shown in the visual check and listed +in the report and the commit body. + +Reviewers read every line of the result. A line that is +copied but does nothing, a bare element selector or a raw z-index comes +back as a review comment, so the rules below are strict about them. + +## Input + +- The path to one `.scss` file (e.g. `frontend/src/app/main/ui/foo/bar.scss`). +- If no path is given, ask for it. Do not pick a file yourself. + +## Workflow + +1. Read the target `.scss` file and the `.cljs` file with the same name next + to it. +2. Read every deprecated source the file uses, so you inline real values, + never remembered ones: + - `frontend/resources/styles/common/refactor/mixins.scss` + - `frontend/resources/styles/common/refactor/design-tokens.scss` + - `frontend/resources/styles/common/refactor/spacing.scss` + - any other file under `frontend/resources/styles/common/refactor/` that + the stylesheet pulls in (placeholders, borders, z-index, fonts...). +3. Read the DS modules you will map to: + - `frontend/src/app/main/ui/ds/_utils.scss` (`px2rem()`) + - `frontend/src/app/main/ui/ds/spacing.scss` + - `frontend/src/app/main/ui/ds/_sizes.scss` + - `frontend/src/app/main/ui/ds/typography.scss` (`use-typography()`) + - `frontend/src/app/main/ui/ds/_borders.scss` + - `frontend/src/app/main/ui/ds/z-index.scss` +4. Apply the rules below and edit the file in place. +5. Verify (see **Verification** and **Visual check**). +6. Report (see **Output**). Do not commit. When the refactor goes with other + changes, it belongs in its own commit, after the task's commits. + +## Primary goal + +After the refactor, the SCSS file must not depend on anything under +`frontend/resources/styles/common/refactor/`. This includes imports, mixins, +placeholder selectors, functions, deprecated variables and design-token +aliases. + +### Partial refactor + +Some placeholders stand for a whole component family that will move to the DS +in its own PR: `%input-element`, `%input-with-label`, `%input-checkbox`, +`%modal-*`, `%menu-dropdown`, `%menu-item-base`, `%shortcut-*` and similar. +Inlining them in a small task produces big, risky diffs. In that case: + +- migrate everything else (tokens, sizes, spacing, small mixins); +- keep the `refactor/` import only for those placeholders; +- list each remaining dependency and the selectors that use it in the report, + so the user can plan it as a separate PR. + +## General rules + +- Keep the resulting SCSS as small and readable as possible. +- Avoid duplicated declarations and duplicated selectors. +- Do not change class names. +- Do not change DOM structure. +- Do not change specificity unless required. +- Follow the project's SCSS formatting (Prettier, see **Verification**). + +## Imports + +Use the DS modules. Import them with `as *` when they expose mixins, +variables or functions: + +```scss +@use "ds/_utils.scss" as *; +``` + +not + +```scss +@use "ds/_utils.scss" as utils; +``` + +Do not introduce namespaces such as `utils.`, `typography.`, `borders.`, +`spacing.` or `sizes.` unless a module explicitly requires one. + +Remove every unused import. + +## Deprecated mixins + +If the stylesheet uses mixins from +`frontend/resources/styles/common/refactor/mixins.scss`: + +- locate the mixin; +- inline its generated CSS (then apply the rest of these rules to it); +- remove the `@include`; +- remove the import. + +Do not leave deprecated mixins. + +## Placeholders + +Replace every `@extend %placeholder` with the generated CSS. Remove every +deprecated placeholder, except the component-family ones described in +**Partial refactor**. + +Inline only what does something on that element. Do not paste a +placeholder (or mixin) body as it is. For each property, check: + +- **It applies to the element.** `fill`, `stroke` and `stroke-width` only + act on SVG shapes: drop them from a `span`, `div` or `button`. + `color: transparent` on a wrapper that only holds an icon does nothing + useful: drop it and set the real color. +- **It is not the default or already given.** `stroke-width: 1px` is the + SVG default; `fill: none` and `stroke: currentColor` are already set by + the DS `icon*` (`.icon`). Drop them. +- **A later rule does not override it.** +- **It is not a stroke width on a DS icon.** DS icons keep the DS + stroke width; drop any `stroke-width` that only imitates the legacy + icon (1.33px, 2.25px...). + +`%button-icon` is the usual case: on a wrapper it leaves `color: +transparent; fill: none; stroke-width: 1px`, and none of the three is +needed. + +## Element selectors + +Do not style HTML elements directly (`svg {}`, `h3 {}`, `button {}`, +`input.x {}`, or `.component { svg {} }`). + +When no class exists, **add one in the `.cljs`**: a DS component takes +`:class (stl/css :x)` (`icon*`, `heading*`, `button*`…), and a plain +element takes `{:class (stl/css :x)}`. Touching the `.cljs` for this is +part of the refactor. A comment like "the icon has no class" is not an +answer. + +Prefer the DS component when one fits: `heading*` for `h1`–`h6` +(`:level`, `:typography`), `text*` for text, `icon*` for icons. + +`element.class` (e.g. `input.element-title`) is not needed to beat the +input defaults: try the class alone and check it in the visual check. + +An element selector stays only for markup you cannot reach (injected +HTML, third-party widgets), with a comment saying so. + +**Icon colors.** A DS icon draws with `stroke: currentColor`. Set +`color` on the icon class or its parent, never `stroke`/`fill` on an +`svg` selector. + +## Color variables + +Whenever a color changes across states, introduce a component-local CSS +variable. + +Instead of + +```scss +.button { + color: var(--color-foreground-primary); + + &:hover { + color: var(--color-accent-primary); + } + + &:disabled { + color: var(--color-foreground-disabled); + } +} +``` + +write + +```scss +.button { + --button-foreground-color: var(--color-foreground-primary); + + color: var(--button-foreground-color); + + &:hover { + --button-foreground-color: var(--color-accent-primary); + } + + &:disabled { + --button-foreground-color: var(--color-foreground-disabled); + } +} +``` + +Use this pattern for `color`, `background-color`, `border-color`, +`outline-color`, `fill`, `stroke` and any similar property. + +## Design token aliases + +Remove every alias defined in +`frontend/resources/styles/common/refactor/design-tokens.scss`. Resolve each +alias to its final DS token. Do not keep intermediate aliases. + +Instead of `color: var(--button-foreground-hover);` write +`color: var(--color-accent-primary);`. + +## Spacing + +Replace deprecated spacing variables using +`frontend/src/app/main/ui/ds/spacing.scss` for `margin`, `padding`, `gap` and +`inset`. + +If no exact spacing token exists, use `px2rem()`. Do not approximate with the +closest token. + +## Sizes + +Replace deprecated size variables using +`frontend/src/app/main/ui/ds/_sizes.scss` for `inline-size`, `block-size`, +`width`, `height` and min/max sizes. + +If no exact size exists, use `px2rem()` from +`frontend/src/app/main/ui/ds/_utils.scss`. Do not leave hardcoded pixel values +unless absolutely necessary. + +## Typography + +When any of `font-size`, `font-weight`, `line-height`, `font-family` or +`letter-spacing` appear, check whether together they match one of the +definitions in `frontend/src/app/main/ui/ds/typography.scss`. + +If they match, replace all the individual declarations with: + +```scss +@include use-typography(""); +``` + +If there is no exact match, use the **closest DS typography** (same role: +a panel title in uppercase → `headline-small`, body text → `body-*`), +even when more than one detail differs. Show the change in the visual +check and state it in the report and in the commit body (e.g. "11px +regular → 12px medium"). + +Keep the original declarations only when no DS typography fits the role +(e.g. a 32px display number in a 14px-only area); then leave a TODO +naming the closest one and ask the user. + +## Borders + +Replace deprecated border radius and border width values using +`frontend/src/app/main/ui/ds/_borders.scss`. A raw width close to a token +(`1.75px`) takes the token (`$b-2`); say so in the report. If nothing is +close, use `px2rem()`. + +## Logical properties + +Prefer logical properties whenever they preserve the existing behavior: + +- `width` → `inline-size` +- `height` → `block-size` +- `max-width` → `max-inline-size` +- `margin-left` → `margin-inline-start` +- `padding-right` → `padding-inline-end` +- `top` → `inset-block-start` +- `left` → `inset-inline-start` + +## Z-index + +Never leave magic z-index values. Use tokens from +`frontend/src/app/main/ui/ds/z-index.scss`, and pick the token **by what +the element is, not by its number**: a dropdown, menu or select → +`--z-index-dropdown`; a toolbar, palette, sidebar or panel → +`--z-index-panels`; and so on (read the comments in that file). A raw `4` +on a dropdown becomes `var(--z-index-dropdown)` even though the number +changes. Check the visual check for overlap. + +Leave a TODO only when the element fits no token's purpose. + +Legacy z-index values between sibling elements that do not overlap can be +removed, but only when the **Visual check** shows no pixel change in every +state that matters (open menus, collapsed, hidden). Say so in the report. + +## Duplicated properties + +Within the same selector, avoid duplicated properties, duplicated CSS +variables and duplicated declarations introduced by the refactor. Allow +duplicates only when they are intentional browser fallbacks. + +## Unused classes + +Open the `.cljs` file with the same name as the SCSS file and check whether +every CSS class is referenced (as `(stl/css :class-name)`, +`(stl/css-case :class-name ...)` or similar). + +CSS modules prefix every class with its module +(`main_ui___`), so only the `.cljs` of the same module can use +it. Before calling a selector unused, also search the whole `frontend/src` +for the class name as a string, a `:global` use, or a dynamic class built +at runtime. + +If a selector is unused after that check: + +- delete it (do not leave it commented out, do not add a TODO); +- list every deleted selector in the report and in the commit body. + +A rule that matches nothing because the markup changed (e.g. `.icon svg` +when the DS icon is the `svg` itself) is deleted the same way. Before +deleting a descendant rule, check every element it matched, not only +the one you changed: `.placeholder .icon svg` can also color a second +icon, which then has no color (in the devenv it shows in the debug +yellow). + +Every DS icon needs a color of its own (`color` on its class or on a +parent with a defined color): an icon that inherits nothing shows in +the debug yellow in the devenv. + +Classes used by markup injected from outside (e.g. an external widget) +stay, with a comment saying so. + +Also check the other direction: every class the `.cljs` passes to +`stl/css` / `stl/css-case` must exist in the `.scss`. A missing one silently +does nothing; report it. + +## Sass and CSS pitfalls + +- Sass does not evaluate functions inside CSS custom properties. Write + `--button-height: calc(var(--height) - #{px2rem(16)});`, not + `... - px2rem(16))`, or the browser gets the literal `px2rem(16)`. +- A `/` between Sass size variables is read as a division + (`grid-template: $sz-32 / auto` breaks). Split it into + `grid-template-rows` / `grid-template-columns` (or areas). +- A CSS-wide keyword cannot be part of a shorthand: `inset-inline: unset 0` + is invalid and stylelint does not catch it. Use the longhands. +- After editing, check the compiled CSS for any of these + (`frontend/resources/public/css/main.css`, search the module class). + +## Uncertainty + +If any transformation is uncertain, never guess. Leave a TODO in the code +and explain why in the report. + +## Self-review + +Before the report, read the final diff as the reviewer would and search +the file for these; each hit must be fixed or have a real reason: + +```bash +rg -n 'svg \{|^\s*(h[1-6]|p|hr|input|button|span|a)\b[^-_]|[a-z]\.[a-z-]+ \{' +rg -n 'stroke-width|color: transparent|fill: none|z-index: [0-9]|TODO' +rg -n '[0-9]\.[0-9]+px|[^(]\b[0-9]+px' +``` + +- an element selector → add a class (see **Element selectors**); +- a property that does not apply or is a default → drop it (see + **Placeholders**); +- a raw z-index → token by purpose; +- a typography TODO → closest DS typography; +- a raw px value → token or `px2rem()`. + +## Verification + +Run the project scripts from `frontend/` inside the devenv container +(`penpot-devenv-ws-main` for workspace `wsN`; never run `pnpm` on the +host), writing output to a file first (never pipe lint output into +filters): + +```bash +docker exec -u penpot -w /home/penpot/penpot/frontend penpot-devenv-ws-main \ + bash -lc 'pnpm run lint:scss; echo LINT=$?; pnpm run check-fmt:scss; echo FMT=$?' \ + > /scss-check.txt 2>&1 +``` + +If the format check fails, run `pnpm run fmt:scss` and check again. + +**Dependency check.** A grep for `refactor/` is not enough. For the final +file, check: + +- `@use` / `@import` sources: all must be `ds/...` (or the sidebar common + module, which is DS-only); +- every `@include`, `@extend` and `%placeholder`; +- every `$variable`: which imported module defines it (`$br-*` exists in both + `ds/_borders.scss` and `refactor/borders.scss`; the `@use` decides); +- every `var(--token)`: defined in the DS (colors, spacing, sizes) or only as a + `refactor/` alias. + +Report any failure as it is; do not hide it. + +## Visual check + +"No visual change" must be shown, not assumed. When a visual spec covers the +component (`frontend/playwright/ui/visual-specs/`, project `ds`), or one can be +added: + +1. Capture before (base branch styles) and after with + `pnpm exec playwright test --project=ds -g "" --update-snapshots=all` + in the container. `=all` is required: plain `--update-snapshots` only + rewrites past the 0.5% tolerance, so a real diff can look like 0. +2. Before each capture, wait until the compiled CSS/JS shows the expected + version (search `main.css` for the changed rule). +3. Compare with zero tolerance. If pixels change, build the before | after | + diff (red) image and explain every change in the report. + +## Output + +1. The diff of the file (`git diff -- `). +2. A summary with: + - removed imports; + - removed deprecated mixins; + - removed placeholders; + - typography replacements; + - design-token aliases removed; + - logical properties introduced; + - deleted unused selectors; + - TODOs requiring manual review, each with its reason; + - remaining `refactor/` dependencies (partial refactor) and where they are + used; + - classes missing from the `.scss`, if any. +3. The verification results, including the visual check (pixel count, or why + it was not possible).