Skip to content

Draw the next-action cue so it shows on a warning notice, and use design system tokens for the window's remaining values - #675

Merged
zaerl merged 2 commits into
trunkfrom
fix/next-action-cue-ring
Oct 9, 2026
Merged

zaerl merged 2 commits into
trunkfrom
fix/next-action-cue-ring

Conversation

@zaerl

@zaerl zaerl commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Why

The old "next thing to do" used hardcoded values that had no space in the WDS, along with other page elements. What the user needs to do should "stand out" on the page, which in CSS is an outline. In our case, it's in the --wpds-color-stroke-surface-brand color.

Before Light Before Dark
immagine immagine
New Light New Dark
immagine immagine

The cue that points a contributor at the next thing to do (#252) could not be seen on the block it points at most often. On the "This site's WordPress code is N days old" notice, the ring was drawn in --wpds-color-stroke-surface-warning, which is the border a warning notice already has, so the ring was just that border again. The rest of shell.css still had a handful of values written by hand where the WordPress Design System has a token, or used a token for a job it is not meant for.

What changes

  • The cue (first commit) is now an outline in --wpds-color-stroke-surface-brand at --wpds-border-width-md, replacing the 3px warning-coloured ring plus a 14px glow mixed with color-mix().
    • Brand, because the next step is guidance and not a warning.
    • Not the strong brand stroke, because that is exactly the focus ring's colour. At the focus ring's width and offset, the cue would look like keyboard focus on a block that does not have it.
    • An outline, so it follows each block's own corners and leaves its border alone.
  • The remaining hand-written values (second commit) now use the design system's tokens, with the same values as before:
    • The status dots are size-4xs and are painted in the foreground role an icon takes (background: currentColor), instead of a surface stroke used as a fill.
    • Hairlines and the conflict rule use the border-width tokens.
    • The step marks and the feedback popover use size tokens.
    • Controls that are not links use --wpds-cursor-control.
    • In the dark theme, the older library's popovers keep only the surface hairline, as the design system's own popover does, and lose the glow I had mixed from the text colour.

Deliberately left out: layout sizes the design system has no token for (breakpoints, column widths, the content width, pane heights, the toast stack's position). Also left out are the overrides of the older @wordpress/components dialogs and popovers, which only go away by moving those dialogs to @wordpress/ui.

How to test this

Platforms: any. Nothing here touches paths, spawning or signing.

Covered by a journey: npx playwright test --project=journeys site-page.spec.js, run from the repository root. The test "a site whose code is old…" now checks that the cue's ring is drawn, is not the colour of the warning notice's border, and is not the focus colour. It is red on trunk's rule and on this branch's first attempt (the strong brand stroke), and green now.

What the journey cannot judge is whether the band looks right, so this is a by-eye check on the current head's Buildkite build.

Starting state: a site whose setup is finished, with its dev server stopped.

  1. Open the site. Expected: the header's "Start development server" button has a soft blue-grey band around it, 4px wide and flush against its edge.
  2. Press Tab until that button has keyboard focus. Expected: the focus ring (crisp blue, 2px, with a small gap) is visibly different from the band, and both can be seen at once.
  3. Start the dev server and wait until it is running. If no ticket is linked, expected: the band moves to the Trac ticket card and follows its rounded corners.
  4. In Settings, switch the theme to Dark and repeat step 1. Expected: the band is still visible.

What must not have happened: the band must not be cut off at any edge, and the cued block must not move or change size when the band appears or goes. The status dots in the sites list and the header must look the same as before, in both themes.

Risks and limitations

  • Custom themes. Under Settings → Custom, a primary colour close to the warning, error or success colour gives a ring the same colour as that notice's border, so it reads as a thicker border. This does not happen with the light, dark or system themes, and on trunk the ring was the warning stroke under every theme. A small outline-offset would separate them whatever the colours; it is not done here.
  • No dark-theme screenshot for the dots. In the light theme the old and new dot tokens are the same values (#936300, #cc1818). For the dark theme I relied on the reviewer's reading of @wordpress/theme's colour ramps, which says the new tokens have at least the old ones' contrast; I did not take a screenshot.

Related

Follow-up to #252.


Design decisions and alternatives considered
  • Strong brand stroke at the focus ring's width. This was the first version on this branch. It is identical to the keyboard focus ring (stroke-surface-brand-strong and stroke-focus are the same step of the same ramp in every theme), so a keyboard user would see two focus rings.
  • Strong brand stroke, only thicker. Rejected because it reads as a heavy focus ring.
  • A box-shadow ring. Rejected: the old rule set the block's border-radius itself so the shadow would curve, which forced the header buttons into 8px corners. A shadow also disappears in forced-colours mode; an outline does not.
Review outcome (required — see AGENTS.md)

0 [fix here] open · 1 [follow-up]. The two earlier [fix here] findings are fixed.

  • Review: completed by a separate agent context (read-only) against .github/instructions/code-review.instructions.md. Head e5cef6f, base 7ee3ead. 2 [fix here]:
    • 🟡 The ring was identical to the keyboard focus ring.
    • 🔵 The journey could not catch that.
    • Both are fixed in e0999a6. The journey assertion goes red on e5cef6f.
  • Since review: e5cef6f → 58f7fd1, re-reviewed by the same agent context with git diff e5cef6f 58f7fd1 plus the clipping and placement checks again. Both earlier findings are resolved. 1 new [follow-up]: 🔵 the custom-theme case above, deferred because it is narrow and still an improvement on trunk.
  • Deterministic layer:
    • eslint . --ignore-pattern '.claude/worktrees/**' and stylelint "src/renderer/**/*.css" are clean. Plain npm run lint also lints the local .claude/worktrees checkouts, which are not part of the repository.
    • npm test: 2096 passed, 2 skipped.
    • All journeys: 149/149 on the token commit. The site-page, pr-checkout and i18n journeys: 40/40 on 58f7fd1.
  • CodeRabbit: completed on 58f7fd1 (base 7ee3ead), requested with @coderabbitai review. No actionable comments. It also noted the custom-theme case already listed under Risks.
Screenshots or recording

The visible change is the cue's ring, shown on the warning notice with the notice's own button focused. I'll attach the screenshots in a comment, since images cannot be added to a PR description from the command line.

🤖 Generated with Claude Code

zaerl and others added 2 commits October 9, 2026 09:57
…ng notice

The cue was a ring in the warning surface stroke, which is the border a
warning notice already has: on the "code is old" notice, the block it points
at most often, the ring was that border again and could not be seen. It also
said "warning" about what is guidance, and was made of a 3px ring and a 14px
glow mixed with color-mix(), neither of them on the design system's scales.

It is now an outline in the brand surface stroke at the system's medium
border width. Brand, because the next step is guidance. Not the strong brand
stroke: that is the focus ring's colour, and at the focus ring's width and
offset the cue would be keyboard focus to look at, on a block or around a
header button that does not have it. An outline, so it follows the block's
own corners and leaves its border alone, which also fits it to the header's
buttons.

The journey checked only that the class was there. It now checks that the
ring is drawn, is not the colour of the notice's border, and is not the
focus colour; red on both earlier rules.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oles

What shell.css still wrote by hand where the design system has a token:

- The status dots of the sites list and the processes are the 4xs size, and
  are painted in the foreground role an icon takes, filled with currentColor,
  instead of a surface stroke used as a fill. The values are the same.
- Hairlines and the conflict rule are the xs and sm border widths, and the
  tabs bar's -1px is minus the xs width.
- The step marks are the 2xs size and the feedback popover the sm surface
  width.
- Controls that are not links take the control cursor.
- In the dark theme the older library's popovers keep only the surface
  hairline, as the design system's own popover draws its edge, and lose the
  glow mixed from the text colour.

What is left untokenised is layout the system has no token for: breakpoints,
column bases, the content width, pane heights and the toast stack's place.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 75ff6fda-637c-464a-935b-4327d5ae9779

📥 Commits

Reviewing files that changed from the base of the PR and between 7ee3ead and 58f7fd1.


📒 Files selected for processing (2)
  • src/renderer/shell.css
  • tests/e2e/journeys/site-page.spec.js

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.



📝 Walkthrough

Walkthrough

Shell styles replace fixed values with design-system tokens for status indicators, borders, dimensions, and interactive cursors. The next-action cue now uses a brand-colored outline, and the dark-mode popover shadow is removed. The site-page test checks the notice outline’s style, width, and color against its border and the focus color token.


Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 58f7f

The shell updates use design-system tokens, and the site-page test targets the updated notice cue. No actionable regression is established; custom themes may make the cue resemble a semantic notice border.

🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check Passed The description is complete and follows the repository template. It explains the motivation, implementation, testing steps, expected outcomes, risks, related issue, design decisions, review status, an…


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zaerl

zaerl commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@zaerl zaerl self-assigned this Oct 9, 2026
@zaerl zaerl added this to the v2.0.0-beta.1 milestone Oct 9, 2026
@zaerl
zaerl merged commit c4b1bfa into trunk Oct 9, 2026
40 checks passed
@zaerl
zaerl deleted the fix/next-action-cue-ring branch October 9, 2026 08:59
zaerl added a commit that referenced this pull request Oct 9, 2026
…dited (#676)

## Why

Since #660, editing a pull request's title or description turns its six
required checks (`unit`, `Journeys` and `Packaged smoke` on macOS and
Windows) into "Expected — Waiting for status to be reported", even
though they already passed on that commit. The merge box shows them as
pending until someone pushes or re-runs. Seen on #675: the runs at 07:59
passed, and the edits at 08:18 and 08:45 started runs
37904200109/37904200065 and 37906918098/37906918097 with every job
skipped.

## What changes

**Root cause:** #660 made `unit-tests.yml` and `e2e.yml` listen for
`edited`, so that a base move would re-run their `changes` job. For any
other edit, `changes` is skipped and every job after it is skipped too.
GitHub treats the newest run of a workflow on a commit as that commit's
checks. A skipped matrix job is reported with its name unexpanded (`unit
(${{ matrix.os }})`), so the required `unit (macos-latest)` and the rest
disappear from the newest run.

**Fix:** both workflows stop listening for `edited`, so a title or
description edit starts nothing in them and the earlier results stand. A
new `.github/workflows/base-change.yml` handles a base move instead. It
finds the latest Unit tests and E2E runs for the PR's head and re-runs
them with `gh run rerun`; a run that is still in progress is cancelled
first and re-run once it stops. A re-run is a new attempt of the same
run rather than a newer run, so it replaces those checks directly. The
`changes` job reads the PR's file list from the API at run time, so the
re-run sees the new base: a Markdown-only stacked PR moved onto trunk is
evaluated as code again, which is the guarantee #660 added.

Deliberately not in it: re-running anything for a fork PR. A fork's
token is read-only, so the workflow posts a warning instead (see Risks).

## How to test this

Platforms: none. This is CI only, and it runs on GitHub's Linux runners.
The workflows only take effect once this is on trunk: `edited` runs use
the workflow files from the PR's merge commit, so a PR opened after this
merges, or one that is pushed to after it merges, picks them up.

**Starting state:** this branch merged to trunk. You need a pull request
against trunk whose head was pushed after the merge, with all six
required checks passed.

1. Edit that PR's description and save. Then edit its title. Expected:
no new run of **Unit tests** or **E2E** appears in the Actions tab.
**Re-run on base change** shows a run in which the `rerun` and `fork`
jobs are both skipped. In the merge box, all six required checks are
still ✓, not "Expected — Waiting for status to be reported".
2. Make a stacked pair: PR A (any code change) against trunk, and PR B
with one Markdown-only commit on top of A's branch, opened against A's
branch. Wait for B's checks. Expected: B passes the six checks through
the stub jobs ("Only Markdown changed").
3. On PR B, choose **Edit** next to the title and change the base from
A's branch to `trunk`. Expected: a **Re-run on base change** run whose
`rerun` job prints `re-running run <id>` for `unit-tests.yml` and
`e2e.yml`. Unit tests and E2E each show a second attempt of their
earlier run. In it, `changes` reports `code=true` (B now carries A's
code), and the real macOS and Windows jobs run in place of the stubs.
4. Optional: repeat step 3 while B's Unit tests run is still in progress
(push a Markdown commit to B, then retarget it straight away). Expected:
the in-progress run is cancelled and then re-run, and the job log shows
both.

**What must not have happened:**

- The six required checks going to "Expected" after step 1. This is the
bug: on trunk today, step 1 reproduces it.
- The stub passes surviving step 3. That would mean B could merge A's
code without the suites having run on it.
- A push made right after a retarget having its new run cancelled. The
job compares the PR's live head with the event's head immediately before
each re-run, and stops if they differ.

**Cannot be tested here:** none of this runs before merge (see
Platforms), and there is no unit test for workflow files. In particular,
it is unverified that `GITHUB_TOKEN` with `actions: write` may cancel
and re-run a `pull_request` run in this repository. Steps 3 and 4 are
the check for that, and the first stacked retarget after merge should be
watched. The workflows pass `actionlint` (with shellcheck), and the `gh
run list` lookup was run against #675's real runs.

## Risks and limitations

- **A re-run tests the old merge commit.** A re-run keeps the original
event, so the suites check out the head merged into the *old* base. Only
the Markdown-only decision uses the new base. That is what #660's
guarantee needs, and it matches every PR run once its base branch has
moved on. #660's `edited` runs did check out a fresh merge with the new
base.
- **Fork PRs:** a fork PR whose base moves is not re-run. A
`::warning::` asks for a manual re-run or a push. `pull_request_target`
would have a write token, but re-running a fork's run with it might
bypass the approval that a first-time contributor's run waits for, so I
did not use it. It is rare: a fork PR based on an upstream branch other
than trunk.
- **Runs older than 30 days:** GitHub refuses to re-run them. The job
fails with an error that says to push, or to close and reopen the PR.
That check is not required, so it does not block the merge on its own.
- **PRs already open when this merges:** where the newest run on the
head is one of the old skipped `edited` runs (as on #675 now),
re-running it replays that skip. A push, or re-running the earlier
passing run from the Actions tab, clears it once.
- Review: 6 [fix here] · 1 [follow-up]. Five fixed; the remaining [fix
here] (no live run yet) is covered by How to test steps 3–4. The
follow-up is the open-PR transition above, noted rather than coded.

## Related

Follow-up to #660. Seen on #675.

---

<details>
<summary>Design decisions and alternatives considered</summary>

- **Keep `edited`, but give the matrix jobs expanded names when
skipped:** not possible. GitHub does not expand a matrix for a job that
is skipped at the job level, which is why #660 added stub jobs in the
first place.
- **Keep `edited` and run the stubs/real jobs again on every edit:**
every title edit would cost two Linux jobs at best and the full
macOS/Windows suites at worst, and it would cancel or duplicate runs.
- **`workflow_dispatch` from the edit workflow:** this would start a
fresh run on the head with the new base. But it is a different event, so
the `pull_request`-only logic in `changes` would need a second branch,
and the checks would come from a run nobody pushed. A re-run reuses
everything that already exists.
- **Concurrency:** base moves share the group `base-change-<PR>`, so a
second move cancels the first and re-runs whatever the first left
behind. Any other edit gets a group per run (`-<run_id>`), so it cannot
cancel a base move. The concurrency group is evaluated before the job's
`if:`, so without this a description edit made during a base move would
have cancelled it.

</details>

<details>
<summary>Review outcome</summary>

6 [fix here] · 1 [follow-up]: 5 fixed, 1 covered by How to test, 1
follow-up recorded in Risks. `npm run lint` clean; unit suite 2096 pass,
0 fail, 2 skipped (local, macOS). `actionlint` clean on all workflows.

- **Review:** completed. Reviewer: a separate agent context, read-only.
Head `da2dbfa`, base `7ee3ead`. Findings:
- 🟡 A description edit could cancel a base-move run, because the
concurrency group is evaluated before `if:`. Fixed.
- 🟡 Re-running the old head's run after a push would cancel the new
head's run through the shared concurrency group. Fixed.
- 🟡 The 30-day re-run limit failed silently and aborted before the
second workflow. Fixed.
  - 🔵 The fork regression was unannounced. Fixed with a warning job.
  - 🔵 [follow-up] PRs open across the merge. Recorded in Risks.
- 🔵 The GitHub semantics are unverified in practice. Covered by How to
test steps 3–4.
- **Since review:** `da2dbfa → 667bd95` re-reviewed by the same separate
context. #1–#4 resolved. One new 🔵: a push during the cancel-and-wait
loop could still be cancelled. Fixed in `667bd95 → 8b5ea25`, which moves
the head check to just before each re-run. I checked that last change
myself; it was not reviewed again.
- **CodeRabbit:** not requested yet.

</details>

<details>
<summary>Implementation notes</summary>

- The run lookup is `gh run list --workflow <file> --event pull_request
--commit <head sha> --branch <head ref> --limit 1`. `--commit` matches
the run's head SHA, not the merge commit's, and `--event` leaves out
trunk push runs.
- The job needs `actions: write` (cancel, re-run) and `pull-requests:
read` (the head check). The workflow's default permissions are `{}`.
Every value from the event reaches the script through `env:`, never
interpolated into it.
- `if ! gh run rerun` keeps a failure for one workflow from skipping the
other under `bash -e`. The job then exits non-zero.
- The `changes` jobs lose their `if:`, and the concurrency groups lose
their `-edit` suffix, since neither workflow sees `edited` any more.

</details>

Screenshots: nothing on screen changed.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant