Repository navigation
feat(icons): add stale, pr-dequeued and lock-outline - #8687
talissoncosta wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21308 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #21308 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-16 — run #21308 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21308 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21264 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21264 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21264 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #21264 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21253 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21253 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21253 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #21253 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
Skipped testsfirefox › tests/onboarding-tests.pw.ts › Onboarding › New user connects via the single-page onboarding flow @oss |
Visual Regression20 screenshots compared. See report for details. |
|
@themis-blindfold review |
55fec31 to
801c8a8
Compare
bdb5b3a to
1439254
Compare
| height={height || width || '16'} | ||
| viewBox='0 0 16 16' | ||
| fill='none' | ||
| stroke={fill || colorIconWarning} |
There was a problem hiding this comment.
🟠 Major · ⚡ Quick win
The stale status icon does not meet the light-theme contrast target.
Observed: without fill, this resolves to #ff9f43; against the default light surface (#ffffff) it is 2.04:1. The icon is intended to carry the stale state on a plain system tag. Predicted: that use would miss the 3:1 non-text contrast requirement. Default it to a contrast-safe warning token (the light colorTextWarning value is #9f5208) while preserving fill overrides.
There was a problem hiding this comment.
Confirmed, and it is worse than this. The stroke is colorIconWarning, not a literal, and that token is #ff9f43 in both themes:
| on white | on dark surface | |
|---|---|---|
today #ff9f43 |
2.04:1 FAIL | 8.82:1 PASS |
in #8594 #ffbc05 |
1.69:1 FAIL | 10.66:1 PASS |
So #8594 moves it further from 3:1, and there are four consumers, three of them predating this PR: this icon, TabMenu/TabButton, CohortCsvSync and the warning toast.
Not fixing it here. Swapping one icon to colorTextWarning would paper over a token that fails everywhere it is used, and a text token on an icon is the wrong semantic even though the numbers work. The fix is to make --color-icon-warning theme-aware the way --color-text-warning already is. That belongs in #8594, which owns the colour system, and I will raise it there.
There was a problem hiding this comment.
Confirmed. stale uses the shared icon token at frontend/web/components/icons/Icon.tsx:1337, and that token remains var(--orange-500) at frontend/web/styles/_tokens.scss:159. Keep this open until #8594 makes --color-icon-warning theme-aware and this branch includes that change; colorTextWarning is not an appropriate local substitute.
⚖️ Themis review: 🟠 Fix before mergeThe new stale icon would miss the 3:1 non-text contrast target on the light surface when used with its default colour. The visual catalogue also masks the new default colours, leaving that behaviour without regression coverage despite the passing CI suite.
🟠 Majors
📝 Walkthrough
🧪 How to verify
Product take: This is a small UI building block, but it feeds the tag-accessibility work directly. 🧭 Assumptions & unverified claimsNo unverified assumptions or claims. A tiny icon, a not-so-tiny contrast ratio · reviewed at bdb5b3a |
801c8a8 to
2f6fb21
Compare
1439254 to
1335119
Compare
…raft The icons the system tags and the permanent padlock need. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2f6fb21 to
a09bf6d
Compare
1335119 to
9557ffe
Compare
docs/if required so people know about the feature.Changes
Contributes to #8465
2/4, splitting #8613. Stacked on #8686.
Adds
stale,pr-dequeuedandlock-outline, and givespr-draftthe colour it was missing. These are the icons the system tags and the permanent padlock need in #8689.Nothing consumes them yet.
How did you test this code?
Typecheck at main's baseline, 916 errors both sides.
Visual check:
npm run storybookstale,pr-dequeuedandlock-outlinerender, andpr-draftis no longer colourless, in light and dark