ALFMOB-271: Source ThemedButton styling from semantic design tokens#92
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates SharedUI’s ButtonTheme to source default/disabled colors from the semantic Theme.button* (and link) token layer, aligning button styling with the design system’s semantic tokens while keeping the public ThemedButton API unchanged.
Changes:
- Repoint
ButtonThemeprimary/secondary/tertiary default+disabled colors fromPrimitives.Colours.*to semanticTheme.button*tokens, and map underline text toTheme.linkLink*tokens. - Tokenize
ThemedButtonicon sizing viaSizing.iconsIconSmalland document remaining literals. - Add
ButtonThemeTeststo assert each theme variant’s spec matches the intended token constants.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Alfie/AlfieKit/Tests/SharedUITests/ButtonThemeTests.swift | Adds unit tests that assert each ButtonTheme variant resolves to the intended semantic tokens/primitives. |
| Alfie/AlfieKit/Sources/SharedUI/Theme/Buttons/ThemedButton.swift | Tokenizes icon size and documents non-tokenized constants; introduces a stroke-width comment near the border overlay. |
| Alfie/AlfieKit/Sources/SharedUI/Theme/Buttons/ButtonTheme.swift | Switches button spec mappings to semantic Theme.* tokens for default/disabled states (and underline text). |
| // No dedicated button-height token exists; snapped down to the nearest spacing-scale values | ||
| // (36→32, 44→40, 52→48) until the design system provides height tokens. | ||
| static let smallHeight: CGFloat = Primitives.Spacing.spacing32 | ||
| static let mediumHeight: CGFloat = Primitives.Spacing.spacing40 |
There was a problem hiding this comment.
High severity — button heights snapped below the 44pt accessibility minimum.
smallHeight/mediumHeight/bigHeight were changed from 36/44/52 to 32/40/48 to fit the spacing-token scale. mediumHeight (the app's default button size) now renders at 40pt, under Apple's Human Interface Guidelines minimum recommended tap target of 44×44pt.
This also reverses a decision made earlier in this same PR's plan/grill docs, which explicitly left heights as documented literal gaps because no height token exists — this commit silently overrides that with no updated AC, no test coverage, and no visual verification (snapshot tests are disabled repo-wide).
Could you check with Thais (design) on whether shrinking every button below the HIG minimum tap target is intentional before this merges? If it is, .medium should probably map to a 44pt-equivalent spacing token rather than 40, to stay HIG-compliant.
There was a problem hiding this comment.
Thanks — good catch on the tap-target. Team decision: keep the token-snapped heights but document the risk with a FIXME (6accca7) noting .medium at 40pt is below the 44pt HIG minimum, pending a proper control-height token. Khoi is confirming the smaller heights with Thais (design); if not signed off we'll add a 44pt-equivalent height token or revert. Proceeding to merge on that basis.
| static let smallHeight: CGFloat = Primitives.Spacing.spacing32 | ||
| static let mediumHeight: CGFloat = Primitives.Spacing.spacing40 | ||
| static let bigHeight: CGFloat = Primitives.Spacing.spacing48 |
There was a problem hiding this comment.
Unlike the Badge (#93) / Chip (#94) PRs, this one isn't a visual no-op: mapping the button heights onto Primitives.Spacing.* shrinks every button by 4pt (36→32 / 44→40 / 52→48). Spacing tokens aren't sizing tokens, and this also contradicts the (good) decision right here to keep borderSelected/close-icon as documented literals.
Suggestion: keep the heights as literal constants annotated "no height token yet" rather than change the rendered size to fit a token — or get design sign-off on the smaller heights.
(Separately: Secondary/Tertiary background goes opaque-white → transparentTransparent — fine on today's white surfaces, will show through on any non-white surface. Fine to leave, just noting.)
There was a problem hiding this comment.
Agreed spacing tokens aren't sizing tokens — there is genuinely no height/size token in the design system (checked all generated files + source token JSONs). Documented with a FIXME (6accca7) and Khoi will confirm the heights with design; we'll swap to a real control-height token once it exists. Noted on the transparent Secondary/Tertiary background too.
6accca7 to
f26f903
Compare
Ticket
ALFMOB-271
Summary
ButtonThemeSpecDefault/Disabled colors (Primary/Secondary/Tertiary) from rawPrimitives.Colours.*onto the semanticTheme.button*token layer.Theme.linkLink*tokens; pressed state and Underline bg/border stay onPrimitives.Colours.*(no semantic token exists — commented).iconSize→Sizing.iconsIconSmall(== 16); document the button-height and 1pt-stroke literals that have no design token.ButtonThemeTestspinning each variant's spec to its intended tokens (snapshot suite is disabled repo-wide — out of scope here).ThemedButtonAPI unchanged; no call-site edits.Note: adopting the semantic tokens intentionally shifts some neutral shades (disabled 100→300 / 400→500, Secondary stroke 800→900, Secondary/Tertiary bg opaque-white→transparent) — identical on today's white surfaces.
Screenshot