feat(ui): sync colour tokens, and extract the token/icon workflows into skills - #182
Open
renefloor wants to merge 15 commits into
Open
feat(ui): sync colour tokens, and extract the token/icon workflows into skills#182renefloor wants to merge 15 commits into
renefloor wants to merge 15 commits into
Conversation
Icon work has enough load-bearing constraints that a prose section in CLAUDE.md could not carry them: upstream groups icons by component ownership rather than subject, so a name has to be searched across core, chat and video; glyph code points are append-only, so deleting an SVG silently repoints every icon after it; and the multicolour file-type assets follow a separate path with none of those rules. The skill covers finding an icon upstream, the size and naming conventions, RTL mirroring, retiring an icon through deprecated.txt and fix_data.yaml, and the file-type assets. CLAUDE.md and STYLE_GUIDE.md keep a short description of what to expect and point at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Syncing a colour token from design-system-tokens is not a copy: this package vendors only the root semantics, and the rules for what to vendor, what to name it and how a default should resolve were nowhere written down. The skill records those rules. A StreamColorScheme field drops the core/utility group segment. Its default resolves through the generated chrome/brand swatch when the token aliases one — a baked hex there silently ignores a custom brand seed — through another field when the token aliases another semantic, and through a vendored constant only for a raw hex. Upstream's derived tokens are deliberately absent, re-derived instead in each component's defaults. Two scripts carry the parts worth automating. flatten_tokens.py diffs the flattened semantics, because the generated output is key-sorted and a re-sort buries real value changes in hundreds of moved lines — which is how a live accent/warning change once reached review unnoticed. map_token_usage.py maps a field to the files that read it in any repo at any ref. It also covers tracing impact into the SDKs, since a namespace does not tell you which repo to open: a tokens/video change can be implemented only by a core component here, and chat components live in this repo outright. Derived tokens cannot be found that way at all, so references/derived-token-map.md holds those traces by hand. Finally it documents coordinating a change across repos through a git dependency override on a shared branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds StreamColorScheme.backgroundOnElevation and borderOnElevation for the new background/core/on-elevation and border/core/on-elevation tokens. Both alias the chrome scale, so the defaults resolve through the generated swatch rather than a vendored constant — a baked hex there would ignore a custom chrome seed. They step up in dark mode, where an elevated surface has already lightened and the plain surface and border values lose their edge against it. Also moves accent/warning from orange to yellow, which upstream changed in the same commit without mentioning it in the PR description: #F26D10 to #F6BF57 in light, #FA922B to #FCD579 in dark. borderWarning defaults to accentWarning so it moves too. The visible effect is StreamErrorBadge's warning style, whose icon is already pinned to black — the new value only carries dark foregrounds, at roughly 1.7:1 against white. The ci/ goldens for that badge need regenerating through the update-goldens workflow; the 56 failing macos goldens here are pre-existing local drift, unchanged by this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two rules learned wiring the on-elevation and call-control-badge tokens through to stream-video-flutter. Always pin a commit SHA in the git override, never a branch name: a branch ref resolves to whatever the tip is at pub get time and writes that SHA into pubspec.lock, so the locked version drifts as the branch moves and two checkouts of the same consumer commit can resolve differently. This replaces the previous advice to use the branch name as the ref, and with it the claim that the branch must outlive its PR. Also: repoint only the package the change touches. stream_core and stream_core_flutter come from this repo but are separate git dependencies with separate pins, and a consumer is often pinned to an older core than main. And a token for an internal component is a signal that the component belongs to the design-system surface, so it usually wants to be public with a Stream prefix — but that widens the public API, so implement the theme and ask rather than deciding it inside a token sync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
Collaborator
Author
|
Companion: GetStream/stream-video-flutter#1338 — themes the call control badge from |
Records the update_goldens workflow as the way to verify a token change. It runs on ubuntu, which is what makes the images match CI, and commits every changed PNG back to the dispatched branch — so the bot's diff doubles as a before/after of every component the color touched, which is worth attaching to a PR when the change is a value move. Also states plainly that every macOS-variant golden fails locally whatever you change, because only the ci/ images are committed and there are no macos/ ones at all. That noise reads as breakage; the way to tell them apart is to revert the edit and watch the same tests fail. Two caveats worth knowing: the regeneration step is continue-on-error, so a green run does not mean the goldens rebuilt cleanly, and the workflow commits to whatever ref is dispatched. Normalizes both skills to American English (colour -> color). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing in the repo said which design-system-tokens state the vendored tokens correspond to, and unlike a token's readers that fact cannot be recovered by grepping — you would have to bisect upstream comparing values. So the reference file now opens with the last synced commit, and the skill points at it: the next sync is a diff from there to origin/main rather than a hunt for the right base. Stated as "every semantic change up to here has been triaged", not "the vendored files mirror this commit", because they do not: 459 names are shared with 4ef9b54, 50 exist only here from before upstream's namespace split, and 98 only upstream, mostly derived tokens this package deliberately skips. Renames the file's title to match what it now holds — sync state and the derived-token traces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xsahil03x
reviewed
Sep 10, 2026
| ), | ||
| shape: OutlinedBorder.lerp(a.shape, b.shape, t), | ||
| side: WidgetStateBorderSide.lerp(a.side, b.side, t), | ||
| side: t < 0.5 ? a.side : b.side, |
Collaborator
Author
There was a problem hiding this comment.
The generated files in my commit came from theme_extensions_builder 7.5.0; the committed ones were produced by an older 7.x.
The old line threw: WidgetStateBorderSide.lerp returns WidgetStateProperty<BorderSide?>?, concretely a _LerpSides, which only implements WidgetStateProperty — it is not a WidgetStateBorderSide. But StreamCheckboxStyle's constructor does side = side as WidgetStateBorderSide?. So the old generated line built a value the constructor could never accept.
I've updated the minimum dependency for theme_extensions_builder, so it's not reverted
5a5ea35
Collaborator
Author
There was a problem hiding this comment.
Following up on the dependency half of this, because I've just changed it back in b71e4ff.
Raising the floor to 7.5.0 made legacy_version_analyze fail: 7.5.0 requires Dart >=3.13.0, while this package declares sdk: ^3.12.0 / flutter: >=3.44.0, so the workspace could not resolve on N-1 stable. That job exists to catch exactly this — packages still analyzing on the previous stable if the package claims to support that version.
The claim is right, so the constraint was the thing to change: theme_extensions_builder is a dev_dependency, so nothing about a consumer on Dart 3.12 is affected. It is back to ^7.2.0, which resolves 7.5.0 on a current SDK (confirmed in the lockfile) and an older 7.x on 3.12 — so the committed generated output is still what a contributor reproduces today.
That leaves the risk the floor was guarding: someone regenerating on an old SDK and silently reinstating the crash. test/theme/component_style_lerp_test.dart now covers it. Worth noting why nothing caught it before — the only lerp test went through StreamTheme.lerp, and every component theme defaults its nested style to null, so StreamCheckboxStyle.lerp(null, null, t) short-circuits before reaching the side branch. The new test builds two styles with a non-null side and fails on main with the original type '_LerpSides' is not a subtype of type 'WidgetStateBorderSide?' in type cast.
A test fails for everyone; a constraint only protects whoever already has the right SDK.
xsahil03x
reviewed
Sep 10, 2026
Comment on lines
+10
to
+25
| - Added `StreamColorScheme.backgroundOnElevation` and `borderOnElevation`, the | ||
| design system's `background/core/on-elevation` and `border/core/on-elevation` | ||
| tokens. For controls sitting inside a floating surface — a menu, dialog or | ||
| popover — where the plain surface and border values lose their edge in dark | ||
| mode, because the elevated surface has already lightened. | ||
|
|
||
| ### 🔄 Changed | ||
|
|
||
| - `StreamColorScheme.accentWarning` moved from orange to yellow, following the | ||
| design system's `accent/warning` token now resolving to `yellow.200` in light | ||
| and `yellow.150` in dark: `#F26D10` → `#F6BF57` (light) and `#FA922B` → | ||
| `#FCD579` (dark). `borderWarning` defaults to `accentWarning`, so it moves with | ||
| it. The visible effect is `StreamErrorBadge` with | ||
| `StreamErrorBadgeStyle.warning`, whose icon is already pinned to black; anything | ||
| overriding either field is unaffected. The new value is legible under dark text | ||
| only — white on `#F6BF57` is roughly 1.7:1 — so pair it with a dark foreground. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 68.02% 68.57% +0.55%
==========================================
Files 209 210 +1
Lines 8554 8593 +39
==========================================
+ Hits 5819 5893 +74
+ Misses 2735 2700 -35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
stream-chat-flutter and stream-video-flutter each ship the same update_goldens.yml, and a value change moves their goldens as surely as this package's — a component they own paints with the field. The call control badge going red to yellow moved call_control_button and call_feature_button, neither of which lives here, and the signal was their CI failing on the variant: CI goldens. Dispatch it on the consumer's branch once its override points at the core commit, so the images rendered are the ones the change actually produces. Also states what belongs in the vendored files: a constant only when a field will read it, in both modes, because an unread constant reads as an invitation to paint a component from a token — which bypasses the seedable color scheme and breaks a custom brand. Dimensions and type never belong there; they come from StreamSpacing, StreamRadius and StreamTokensTypography. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Said dimensions and typography were not synced from upstream at all. Wrong: the vendored files mirror upstream's values exactly, ordering included, and StreamSpacing / StreamRadius / StreamTokensTypography hold the same numbers. Typography specifically comes from the web flavor, which is the one place the Geist family this package ships appears — android resolves to Roboto, iOS to SF Pro, and iOS also runs a size up at every step. So "pick any flavor" holds for colors and is wrong for type. The dimension values are identical across all three flavors, so no choice arises there, but the classes hard-code them rather than reading the vendored constants — the same hazard as baking a hex where a swatch belongs, one layer up, since nothing ties the class to the constant it mirrors. Also finishes the American English pass: the earlier sweep only covered colour and left flavour behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
renefloor
added this pull request to stack #185
September 10, 2026 12:29
Both new entries ran to a paragraph where their neighbours run to a line. Reviewer called it too verbose, and rightly — the changelog is scanned by consumers deciding whether an upgrade affects them, so the values belong there and the reasoning belongs in the PR. Adds the rule where it will be seen next time: the changelog policy in STYLE_GUIDE.md, and the skill's changelog section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The regenerated theme files in this branch come from 7.5.0, which replaced `WidgetStateBorderSide.lerp(a, b, t)` with a midpoint step. That is a fix, not a downgrade: `lerp` returns a `_LerpSides`, which only implements `WidgetStateProperty<BorderSide?>`, while the style constructors cast to `WidgetStateBorderSide?` — so the old output threw `type '_LerpSides' is not a subtype of type 'WidgetStateBorderSide?'` on any theme animation where both endpoints set a side. At `^7.2.0` an older builder could regenerate and put the crash back, so the floor moves to the version that emits the fix. The constraint lives in melos.yaml, which bootstrap propagates into the package pubspec — editing the pubspec alone is reverted on the next bootstrap. Regenerating at the new floor produces no diff, so the committed output already matches it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
renefloor
marked this pull request as ready for review
September 10, 2026 14:03
The dartdoc was upstream's `$description` copied verbatim, and upstream is
wrong: `border/core/on-elevation` resolves to `{chrome.300}` in dark, identical
to `border/core/on-surface`, so nothing "steps up". It is the light mode that
differs, where the field is softer than `borderOnSurface`. Reported upstream.
The `WidgetStateBorderSide` lerp crash had no test — the existing
`StreamTheme.lerp` coverage leaves every nested style null, short-circuiting
before the branch is reached. The new test fails on main with the original
`_LerpSides is not a subtype of WidgetStateBorderSide?` cast error.
Also moves the fix bullet out of `### 🔄 Changed`, which the style guide
reserves for what is neither new API nor a fix, and records the nullable
`BorderSide` endpoint change the regeneration also made.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things were wrong. `stream_color_swatch_helper.dart` was listed as a reader of the vendored tokens; it generates shades in HCT from a seed and never touches `StreamTokens`. "Typography, spacing and radius do not come from upstream at all" is false — they are published upstream and mirrored by hand into `StreamTokensTypography`, `StreamSpacing` and `StreamRadius`, which is what the skill already said, so the two contradicted each other. And the derived-token-map rows for the call-control error badge pointed at the error members for tokens that resolve to warning values. Adds the provenance that was missing: a field's dartdoc is the token's own `$description`, which is why a stale upstream description reaches this repo unexamined. Says to verify it against the resolved values first. Also: the example `ref:` was a commit on this branch and would dangle after squash; `map_token_usage.py` matches Material's `colorScheme` identically, which matters because it is meant to run against the Material-based SDKs; both scripts mangled their own `--help`; and the sync-state counts named no command and could not be reproduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
My earlier commit called upstream's `border/core/on-elevation` description wrong.
It is not. "Steps up in dark mode to keep the edge visible once the elevated
surface has lightened" describes the token's own progression — `{chrome.150}` in
light to `{chrome.300}` in dark, as `background/core/elevation-2` goes
`{chrome.0}` to `{chrome.100}`. I read it as a comparison with `borderOnSurface`
and rewrote accurate wording.
What is true, and what the description leaves out, is that the same `{chrome.300}`
is also `borderOnSurface` in dark — so the "use borderOnSurface on a plain
surface" guidance only changes anything in light. That is now appended rather
than substituted.
The skill's dartdoc section carried the same mistake as its worked example; it
now warns that a description tracks the token's own light/dark step rather than
a comparison with the sibling it names, and to add the missing nuance instead of
replacing wording that is accurate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7.5.0 requires Dart >=3.13.0, while this package declares `sdk: ^3.12.0` and `flutter: >=3.44.0`. That combination cannot resolve, which is what `legacy_version_analyze` exists to catch — it checks the N-1 stable still works "if the package claims to support that version". The claim is accurate: `theme_extensions_builder` is a dev_dependency, so a consumer on Dart 3.12 is unaffected and the published package really does still support it. `^7.2.0` lets pub pick 7.5.0 on a modern SDK (verified in the lockfile) and an older 7.x on 3.12, so the committed generated output is what a current contributor reproduces. The floor was raised to stop someone regenerating with an old builder and silently reinstating the `WidgetStateBorderSide` cast crash. That is now guarded by a test instead, which is the more durable fix — a constraint only protects whoever has the right SDK, a test fails for everyone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Syncs the colour tokens from design-system-tokens#73 (merged as
4ef9b54), and moves the token- and icon-syncing procedures out ofCLAUDE.md/STYLE_GUIDE.mdinto two skills.Tokens
accent/warningmoves from orange to yellow. Worth a look, because it is the one existing value that changes and it is not in the upstream PR description — it appears only in a commit message.accentWarninggoes#F26D10→#F6BF57(light) and#FA922B→#FCD579(dark);borderWarningdefaults to it, so it follows.The visible effect is
StreamErrorBadge's warning style from #180, whose icon is already pinned to black. That pairing matters: the new value carries dark foregrounds only — white on#F6BF57is roughly 1.7:1.backgroundOnElevationandborderOnElevationare newStreamColorSchemefields. Both alias the chrome scale, so their defaults resolve through the generated swatch (chrome.shade100/shade150/shade300) rather than a vendored constant — a baked hex there would ignore a custom chrome seed. They step up in dark mode, where an elevated surface has already lightened and the plain surface and border values lose their edge against it.ci/goldens for the error badge need regenerating through the update-goldens workflow. The 56 failingmacosgoldens locally are pre-existing drift — verified by reverting the four values and getting the same failures.Skills
.claude/skills/update-design-tokens/and.claude/skills/update-icons/, with the two docs trimmed to a short description plus a pointer.The token skill records what was previously unwritten: only root semantics are vendored (upstream's
badge/*,button/*,avatar/*are re-derived in component defaults), a field drops thecore/utilitygroup segment, and a default resolves through a swatch, another field, or a vendored constant depending on what the token aliases. It bundles two scripts — one diffs the flattened semantics, because the generated output is key-sorted and a re-sort buries real value changes; the other maps acolorSchemefield to the files that read it in any repo at any ref.Companion PR
Paired with the video-side change, which themes the call control badge from
control/call-control-error-badge/*. Itsstream_core_flutteroverride is pinned to a commit on this branch, so this needs to merge first, and that override then wants repointing at the resultingmaincommit.