ALFMOB-271: Source ThemedButton styling from semantic design tokens #92
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,10 +136,13 @@ private enum Constants { | |
| static let horizontalPadding: CGFloat = 0 | ||
| static let verticalPadding: CGFloat = -Primitives.Spacing.spacing8 | ||
| static let cornerRadius: CGFloat = Sizing.radiusSoft | ||
| static let iconSize: CGFloat = Sizing.iconsIconSmall | ||
| // FIXME: No dedicated button-height token exists; heights are snapped to the spacing scale | ||
| // (36→32, 44→40, 52→48). This drops `.medium` (the default) to 40pt — below Apple's 44pt | ||
| // minimum tap target. Pending design confirmation on adding proper control-height tokens. | ||
| static let smallHeight: CGFloat = Primitives.Spacing.spacing32 | ||
| static let mediumHeight: CGFloat = Primitives.Spacing.spacing40 | ||
| static let bigHeight: CGFloat = Primitives.Spacing.spacing48 | ||
|
Comment on lines
+143
to
+145
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 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 →
Contributor
Author
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. |
||
| } | ||
|
|
||
| // MARK: - ThemedButtonStyle | ||
|
|
@@ -181,7 +184,7 @@ private struct ThemedButtonStyle: ButtonStyle { | |
| .overlay( | ||
| ZStack { | ||
| RoundedRectangle(cornerRadius: Constants.cornerRadius) | ||
| .stroke(borderColor(configuration), lineWidth: Primitives.Border.borderWeightDefault) | ||
|
|
||
| if isLoading { | ||
| LoaderView(circleDiameter: .defaultSmall, style: styleLoading) | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import SwiftUI | ||
| import XCTest | ||
| @testable import SharedUI | ||
|
|
||
| /// Pins each `ButtonTheme` variant to the design tokens it must source colors from, so a future | ||
| /// token regeneration (or an accidental edit) that re-points a mapping is caught. Default/disabled | ||
| /// colors come from the semantic `Theme.button*` layer; the pressed state and the underline | ||
| /// background/border have no semantic token and intentionally stay on `Primitives.Colours.*`. | ||
| final class ButtonThemeTests: XCTestCase { | ||
| func test_primary_sourcesSemanticButtonTokens() { | ||
| let spec = ButtonTheme.primary.spec | ||
| XCTAssertEqual(spec.backgroundColor, Theme.buttonPrimaryBackgroundPrimaryDefault) | ||
| XCTAssertEqual(spec.backgroundDisabledColor, Theme.buttonPrimaryBackgroundPrimaryDisabled) | ||
| XCTAssertEqual(spec.textColor, Theme.buttonPrimaryContentPrimaryDefault) | ||
| XCTAssertEqual(spec.textDisabledColor, Theme.buttonPrimaryContentPrimaryDisabled) | ||
| XCTAssertEqual(spec.borderColor, Theme.buttonPrimaryStrokePrimaryDefault) | ||
| XCTAssertEqual(spec.borderDisabledColor, Theme.buttonPrimaryStrokePrimaryDisabled) | ||
| // Pressed: no semantic token — primitives retained. | ||
| XCTAssertEqual(spec.backgroundPressedColor, Primitives.Colours.neutrals500) | ||
| XCTAssertEqual(spec.textPressedColor, Primitives.Colours.neutrals0) | ||
| XCTAssertEqual(spec.borderPressedColor, Primitives.Colours.neutrals500) | ||
| } | ||
|
|
||
| func test_secondary_sourcesSemanticButtonTokens() { | ||
| let spec = ButtonTheme.secondary.spec | ||
| XCTAssertEqual(spec.backgroundColor, Theme.buttonSecondaryBackgroundSecondaryDefault) | ||
| XCTAssertEqual(spec.backgroundDisabledColor, Theme.buttonSecondaryBackgroundSecondaryDisabled) | ||
| XCTAssertEqual(spec.textColor, Theme.buttonSecondaryContentSecondaryDefault) | ||
| XCTAssertEqual(spec.textDisabledColor, Theme.buttonSecondaryContentSecondaryDisabled) | ||
| XCTAssertEqual(spec.borderColor, Theme.buttonSecondaryStrokeSecondaryDefault) | ||
| XCTAssertEqual(spec.borderDisabledColor, Theme.buttonSecondaryStrokeSecondaryDisabled) | ||
| XCTAssertEqual(spec.backgroundPressedColor, Primitives.Colours.neutrals100) | ||
| XCTAssertEqual(spec.textPressedColor, Primitives.Colours.neutrals500) | ||
| XCTAssertEqual(spec.borderPressedColor, Primitives.Colours.neutrals500) | ||
| } | ||
|
|
||
| func test_tertiary_sourcesSemanticButtonTokens() { | ||
| let spec = ButtonTheme.tertiary.spec | ||
| XCTAssertEqual(spec.backgroundColor, Theme.buttonTerciaryBackgroundTerciaryDefault) | ||
| XCTAssertEqual(spec.backgroundDisabledColor, Theme.buttonTerciaryBackgroundTerciaryDisabled) | ||
| XCTAssertEqual(spec.textColor, Theme.buttonTerciaryContentTerciaryDefault) | ||
| XCTAssertEqual(spec.textDisabledColor, Theme.buttonTerciaryContentTerciaryDisabled) | ||
| XCTAssertEqual(spec.borderColor, Theme.buttonTerciaryStrokeTerciaryDefault) | ||
| XCTAssertEqual(spec.borderDisabledColor, Theme.buttonTerciaryStrokeTerciaryDisabled) | ||
| XCTAssertEqual(spec.backgroundPressedColor, Primitives.Colours.neutrals100) | ||
| XCTAssertEqual(spec.textPressedColor, Primitives.Colours.neutrals500) | ||
| XCTAssertEqual(spec.borderPressedColor, Primitives.Colours.neutrals100) | ||
| } | ||
|
|
||
| func test_underline_sourcesSemanticLinkTokensForText() { | ||
| let spec = ButtonTheme.underline.spec | ||
| // Underline has no semantic button group → text tracks the semantic link tokens. | ||
| XCTAssertEqual(spec.textColor, Theme.linkLinkPrimaryDefault) | ||
| XCTAssertEqual(spec.textDisabledColor, Theme.linkLinkPrimaryDisabled) | ||
| // Pressed text + the (invisible) background/border have no matching token — primitives retained. | ||
| XCTAssertEqual(spec.textPressedColor, Primitives.Colours.neutrals500) | ||
| XCTAssertEqual(spec.backgroundColor, Primitives.Colours.neutrals0) | ||
| XCTAssertEqual(spec.backgroundDisabledColor, Primitives.Colours.neutrals0) | ||
| XCTAssertEqual(spec.backgroundPressedColor, Primitives.Colours.neutrals0) | ||
| XCTAssertEqual(spec.borderColor, Primitives.Colours.neutrals0) | ||
| XCTAssertEqual(spec.borderDisabledColor, Primitives.Colours.neutrals0) | ||
| XCTAssertEqual(spec.borderPressedColor, Primitives.Colours.neutrals0) | ||
| } | ||
| } |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High severity — button heights snapped below the 44pt accessibility minimum.
smallHeight/mediumHeight/bigHeightwere 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,
.mediumshould probably map to a 44pt-equivalent spacing token rather than 40, to stay HIG-compliant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks — good catch on the tap-target. Team decision: keep the token-snapped heights but document the risk with a FIXME (6accca7) noting
.mediumat 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.