feat(web, web-react): extract color-scheme to helpers and update Pill#2565
feat(web, web-react): extract color-scheme to helpers and update Pill#2565crishpeen wants to merge 3 commits intorelease/v5-essencefrom
Pill#2565Conversation
✅ Deploy Preview for spirit-design-system-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for spirit-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
BREAKING CHANGE: Remove color-scheme from the utilities settings and move generation to helpers/color-scheme. Consumers must import the color-scheme helper entry (or the updated main bundle) instead of relying on utilities-only builds for color-scheme classes. Pill and ControlButton use the new component token override mixin.
crishpeen
force-pushed
the
feat/color-scheme-package-split
branch
from
73604be to
32ae40f
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: feat/color-scheme-package-split
Scope: 3 commits, 33 files (+812 / −266)
What this branch does
- Moves color-scheme utility generation from the utilities map into a dedicated SCSS helper (
helpers/color-scheme) that emits.color-scheme-on-*classes setting--spirit-local-background-colorand--spirit-local-coloronly (no!importanton longhands in the helper path). - Introduces
component-color-overridesto discovercomponent-*tokens and emit high-specificity modifier overrides; updatesPillto consumelocal-*variables and documents optional component token overrides in design-tokens README. - Adds React utilities (
colorTokens,getColorSchemeClassName), wiresPillwithisSubtleand matching color-scheme classes, refactorsTag/IconBoxtocreateScopedColorTokenName, and expands unit/provider tests.
Summary
| Dimension | Notes |
|---|---|
| Security | No concerns in the diff. |
| Bugs / behavior | Pill now defaults isSubtle to false and always applies a color-scheme class; documented for vanilla HTML. |
| Tests | Good coverage for new helpers, Pill, and SCSS (sass-true). |
| Docs / DX | One small wording fix suggested in Pill README (see inline). |
Verdict: APPROVE — No blocking findings; a few non-blocking suggestions and one scope question below.
praise
Solid split between helpers and utilities, shared token naming between TS and SCSS, thorough tests (including sass-true for the new mixins), and clear docs for consumers who must add color-scheme-on-* in plain HTML. The component-color-overrides approach scales better than hand-maintained dictionaries for optional tokens.
Sent by Cursor Automation: Spirit Cursor Code Review
| <span class="Pill Pill--danger color-scheme-on-emotion-danger-basic">3</span> | ||
| ``` | ||
|
|
||
| ℹ️ Even thought the `Pill--<color>` modifier is not used by default, it can be used to override the color scheme when needed. See [Default Color Overrides](#default-color-overrides) for more information. |
There was a problem hiding this comment.
suggestion (non-blocking): Fix the typo “Even thought” → “Even though” so the sentence reads naturally.
Fix: Replace the phrase on this line.
Why: Readers may trip on the grammar; it is a quick doc polish.
| generate-custom-properties: false, | ||
| values: ( | ||
| block, | ||
| contents, |
There was a problem hiding this comment.
question (non-blocking): Was adding contents to the display utility map intentional for this change, or did it ride along from another task?
Why: It is unrelated to the color-scheme / Pill work; if accidental, consider moving it to a separate commit or noting it in the PR description for changelog clarity.
| &:is(:disabled, .is-disabled, .ControlButton--disabled) { | ||
| // TODO This is not ideal, check with señor Adam how to handle this. |
There was a problem hiding this comment.
suggestion (non-blocking, if-minor): Consider replacing or tightening the informal TODO / “señor Adam” notes before merge, or tracking them in an issue linked from the PR so the codebase does not accumulate ambiguous follow-ups.
Fix: Either resolve the specificity/background questions in this PR if the design is settled, or shorten to a neutral TODO(#issue): … with a ticket.
Why: Future readers may not have context; linked issues keep intent clear without blocking the feature.
1 participant
Description
This PR splits color-scheme concerns out of the generic utilities pipeline in
@alma-oss/spirit-web, adds dedicated SCSS helpers and a component token override mixin, and aligns@alma-oss/spirit-web-react(Pill,Tag,IconBox) withgetColorSchemeClassName/colorTokens. Design token documentation indesign-tokensis updated to describe the new setup.Packages:
web(breaking),web-react,design-tokens(docs).Additional context
settings/_utilitiesandtools/_utilitiesintohelpers/color-schemeand related tooling; consumers that depended on utilities-only builds for.color-scheme-on-*must import the color-scheme helper entry or the updated bundle.colorObjectGenerators→colorTokens), added tests and provider coverage for color-scheme props.Issue reference
Link the Jira ticket when available (e.g.
https://jira.almacareer.tech/browse/DS-XXXX).