-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WB-1814.4] Refactor Accordion, Banner, BirthdayPicker to use semantic colors #2446
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
"@khanacademy/wonder-blocks-birthday-picker": patch | ||
"@khanacademy/wonder-blocks-accordion": patch | ||
"@khanacademy/wonder-blocks-banner": patch | ||
--- | ||
|
||
Replace `color` with `semanticColor` tokens |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ import {View} from "@khanacademy/wonder-blocks-core"; | |
import {PhosphorIcon, PhosphorIconAsset} from "@khanacademy/wonder-blocks-icon"; | ||
import IconButton from "@khanacademy/wonder-blocks-icon-button"; | ||
import Link from "@khanacademy/wonder-blocks-link"; | ||
import {color, spacing} from "@khanacademy/wonder-blocks-tokens"; | ||
import {font, semanticColor, spacing} from "@khanacademy/wonder-blocks-tokens"; | ||
import {LabelSmall} from "@khanacademy/wonder-blocks-typography"; | ||
|
||
import infoIcon from "@phosphor-icons/core/regular/info.svg"; | ||
|
@@ -131,26 +131,26 @@ const getValuesForKind = (kind: BannerKind): BannerValues => { | |
switch (kind) { | ||
case "success": | ||
return { | ||
color: color.green, | ||
color: semanticColor.status.success.foreground, | ||
icon: successIcon, | ||
role: "status", | ||
}; | ||
case "warning": | ||
return { | ||
color: color.gold, | ||
color: semanticColor.status.warning.foreground, | ||
icon: warningIcon, | ||
role: "alert", | ||
ariaLive: "polite", | ||
}; | ||
case "critical": | ||
return { | ||
color: color.red, | ||
color: semanticColor.status.critical.foreground, | ||
icon: criticalIcon, | ||
role: "alert", | ||
}; | ||
default: | ||
return { | ||
color: color.blue, | ||
color: semanticColor.status.notice.foreground, | ||
icon: infoIcon, | ||
role: "status", | ||
}; | ||
|
@@ -313,7 +313,9 @@ const styles = StyleSheet.create({ | |
// the base color needs to be hard-coded as white for the | ||
// intended pastel background color to show up correctly | ||
// on dark backgrounds. | ||
backgroundColor: color.white, | ||
// TODO(WB-1865): Verify if we can change this to use semanticColor | ||
// status tokens. | ||
backgroundColor: semanticColor.surface.primary, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohh interesting, we don't use the semantic background colors for the background of the banner (we rely on opacity). Do we want to use the semantic background tokens? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! tbh I'm not sure how this will be handled with the Polaris theme. I'm going to revisit this once we add theming support to banner. I'll add a TODO comment to make sure we track this. |
||
}, | ||
containerInner: { | ||
flexDirection: "row", | ||
|
@@ -328,7 +330,7 @@ const styles = StyleSheet.create({ | |
marginInlineStart: spacing.xxxxSmall_2, | ||
marginInlineEnd: spacing.xSmall_8, | ||
alignSelf: "flex-start", | ||
color: color.offBlack64, | ||
color: semanticColor.icon.primary, | ||
}, | ||
labelAndButtonsContainer: { | ||
flex: 1, | ||
|
@@ -359,7 +361,7 @@ const styles = StyleSheet.create({ | |
justifyContent: "center", | ||
}, | ||
link: { | ||
fontSize: 14, | ||
fontSize: font.size.small, | ||
}, | ||
dismiss: { | ||
flexShrink: 1, | ||
|
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.
note: We now support Safari v14+, so I dropped this.