-
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-1808: Button - Use CSS pseudo-classes for styling states (hover, focus, etc) #2404
base: main
Are you sure you want to change the base?
Changes from all commits
e170282
ff165fa
bcb6ced
b32a14a
ebd5562
df9188e
598be12
3aee270
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,5 @@ | ||
--- | ||
"@khanacademy/wonder-blocks-button": patch | ||
--- | ||
|
||
Use pseudo-classes for styling states (:hover, :focus-visible). Keep some clickable states for programmatic focus and preserve active/pressed overrides. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
import * as React from "react"; | ||
import {StyleSheet} from "aphrodite"; | ||
import {action} from "@storybook/addon-actions"; | ||
import type {Meta, StoryObj} from "@storybook/react"; | ||
|
||
import paperPlaneIcon from "@phosphor-icons/core/fill/paper-plane-tilt-fill.svg"; | ||
import {PropsFor, View} from "@khanacademy/wonder-blocks-core"; | ||
import {ThemeSwitcherContext} from "@khanacademy/wonder-blocks-theming"; | ||
import {color, semanticColor, spacing} from "@khanacademy/wonder-blocks-tokens"; | ||
import {HeadingLarge, LabelMedium} from "@khanacademy/wonder-blocks-typography"; | ||
import Button from "@khanacademy/wonder-blocks-button"; | ||
|
||
/** | ||
* The following stories are used to generate the pseudo states for the | ||
* Button component. This is only used for visual testing in Chromatic. | ||
*/ | ||
export default { | ||
title: "Packages / Button / All Variants", | ||
parameters: { | ||
docs: { | ||
autodocs: false, | ||
}, | ||
chromatic: { | ||
// NOTE: This is required to prevent Chromatic from cutting off the | ||
// dark background in screenshots (accounts for all the space taken | ||
// by the variants). | ||
viewports: [1700], | ||
}, | ||
}, | ||
} as Meta; | ||
|
||
type StoryComponentType = StoryObj<typeof Button>; | ||
|
||
type ButtonProps = PropsFor<typeof Button>; | ||
|
||
const sizes: Array<ButtonProps["size"]> = ["medium", "small", "large"]; | ||
const kinds: Array<ButtonProps["kind"]> = ["primary", "secondary", "tertiary"]; | ||
|
||
const colors: Array<ButtonProps["color"]> = ["default", "destructive"]; | ||
|
||
function VariantsGroup({ | ||
color = "default", | ||
disabled = false, | ||
label = "Button", | ||
light, | ||
size, | ||
}: { | ||
color?: ButtonProps["color"]; | ||
disabled?: ButtonProps["disabled"]; | ||
label?: string; | ||
light: boolean; | ||
size: ButtonProps["size"]; | ||
}) { | ||
const theme = React.useContext(ThemeSwitcherContext); | ||
const category = disabled ? "disabled" : color; | ||
|
||
return ( | ||
<View | ||
style={[ | ||
styles.variants, | ||
light && | ||
(theme === "khanmigo" | ||
? styles.darkKhanmigo | ||
: styles.darkDefault), | ||
]} | ||
> | ||
<LabelMedium style={[styles.label, light && styles.inverseLabel]}> | ||
{size} / {category} | ||
</LabelMedium> | ||
{kinds.map((kind) => ( | ||
<React.Fragment key={kind}> | ||
<Button | ||
onClick={action("clicked")} | ||
disabled={disabled} | ||
kind={kind} | ||
light={light} | ||
color={color} | ||
size={size} | ||
> | ||
{label} | ||
</Button> | ||
{/* startIcon */} | ||
<Button | ||
onClick={action("clicked")} | ||
disabled={disabled} | ||
kind={kind} | ||
light={light} | ||
color={color} | ||
size={size} | ||
startIcon={paperPlaneIcon} | ||
> | ||
{label} | ||
</Button> | ||
{/* endIcon */} | ||
<Button | ||
onClick={action("clicked")} | ||
disabled={disabled} | ||
kind={kind} | ||
light={light} | ||
color={color} | ||
size={size} | ||
endIcon={paperPlaneIcon} | ||
> | ||
{label} | ||
</Button> | ||
</React.Fragment> | ||
))} | ||
</View> | ||
); | ||
} | ||
|
||
const KindVariants = ({light}: {light: boolean}) => { | ||
return ( | ||
<> | ||
{sizes.map((size) => ( | ||
<> | ||
{colors.map((color) => ( | ||
<VariantsGroup | ||
size={size} | ||
color={color} | ||
light={light} | ||
/> | ||
))} | ||
<VariantsGroup size={size} disabled={true} light={light} /> | ||
</> | ||
))} | ||
</> | ||
); | ||
}; | ||
|
||
const VariantsByTheme = ({themeName = "Default"}: {themeName?: string}) => ( | ||
<View style={{marginBottom: spacing.large_24}}> | ||
<HeadingLarge>{themeName} theme</HeadingLarge> | ||
<KindVariants light={false} /> | ||
<KindVariants light={true} /> | ||
</View> | ||
); | ||
|
||
const AllVariants = () => ( | ||
<> | ||
<VariantsByTheme /> | ||
<ThemeSwitcherContext.Provider value="khanmigo"> | ||
<VariantsByTheme themeName="Khanmigo" /> | ||
</ThemeSwitcherContext.Provider> | ||
</> | ||
); | ||
|
||
export const Default: StoryComponentType = { | ||
render: AllVariants, | ||
}; | ||
|
||
export const Hover: StoryComponentType = { | ||
render: AllVariants, | ||
parameters: {pseudo: {hover: true}}, | ||
}; | ||
|
||
export const Focus: StoryComponentType = { | ||
render: AllVariants, | ||
parameters: {pseudo: {focusVisible: true}}, | ||
}; | ||
|
||
export const HoverFocus: StoryComponentType = { | ||
name: "Hover + Focus", | ||
render: AllVariants, | ||
parameters: {pseudo: {hover: true, focusVisible: true}}, | ||
}; | ||
|
||
export const Active: StoryComponentType = { | ||
render: AllVariants, | ||
parameters: {pseudo: {active: true}}, | ||
}; | ||
|
||
const styles = StyleSheet.create({ | ||
darkDefault: { | ||
backgroundColor: color.darkBlue, | ||
}, | ||
darkKhanmigo: { | ||
backgroundColor: color.eggplant, | ||
}, | ||
variants: { | ||
justifyContent: "flex-start", | ||
padding: spacing.medium_16, | ||
display: "flex", | ||
flexDirection: "row", | ||
alignItems: "center", | ||
gap: spacing.xLarge_32, | ||
}, | ||
label: { | ||
minWidth: 150, | ||
}, | ||
inverseLabel: { | ||
color: semanticColor.text.inverse, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
import * as React from "react"; | ||
import {StyleSheet} from "aphrodite"; | ||
import type {Meta, StoryObj} from "@storybook/react"; | ||
import {expect, userEvent, within} from "@storybook/test"; | ||
|
||
import {MemoryRouter, Route, Switch} from "react-router-dom"; | ||
|
||
|
@@ -27,26 +26,6 @@ import ComponentInfo from "../../.storybook/components/component-info"; | |
import ButtonArgTypes from "./button.argtypes"; | ||
import {ThemeSwitcherContext} from "@khanacademy/wonder-blocks-theming"; | ||
|
||
/** | ||
* Reusable button component. | ||
* | ||
* Consisting of a [`ClickableBehavior`](#clickablebehavior) surrounding a | ||
* `ButtonCore`. `ClickableBehavior` handles interactions and state changes. | ||
* `ButtonCore` is a stateless component which displays the different states the | ||
* `Button` can take. | ||
* | ||
* ### Usage | ||
* | ||
* ```tsx | ||
* import Button from "@khanacademy/wonder-blocks-button"; | ||
* | ||
* <Button | ||
* onClick={(e) => console.log("Hello, world!")} | ||
* > | ||
* Hello, world! | ||
* </Button> | ||
* ``` | ||
*/ | ||
export default { | ||
title: "Packages / Button", | ||
component: Button, | ||
|
@@ -80,61 +59,14 @@ export const Default: StoryComponentType = { | |
}, | ||
}, | ||
parameters: { | ||
design: { | ||
type: "figma", | ||
url: "https://www.figma.com/file/VbVu3h2BpBhH80niq101MHHE/%F0%9F%92%A0-Main-Components?type=design&node-id=389-0&mode=design", | ||
}, | ||
chromatic: { | ||
// We already have screenshots of other stories that cover more of the button states | ||
// We already have screenshots of other stories that cover more of | ||
// the button states | ||
disableSnapshot: true, | ||
}, | ||
}, | ||
}; | ||
|
||
export const Tertiary: StoryComponentType = { | ||
args: { | ||
onClick: () => {}, | ||
kind: "tertiary", | ||
testId: "test-button", | ||
children: "Hello, world!", | ||
}, | ||
play: async ({canvasElement}) => { | ||
const canvas = within(canvasElement); | ||
|
||
// Get HTML elements | ||
const button = canvas.getByRole("button"); | ||
const innerLabel = canvas.getByTestId("test-button-inner-label"); | ||
const computedStyleLabel = getComputedStyle(innerLabel, ":after"); | ||
|
||
// Resting style | ||
await expect(button).toHaveStyle(`color: ${color.blue}`); | ||
await expect(button).toHaveTextContent("Hello, world!"); | ||
|
||
// Hover style | ||
await userEvent.hover(button); | ||
await expect(computedStyleLabel.height).toBe("2px"); | ||
await expect(computedStyleLabel.color).toBe("rgb(24, 101, 242)"); | ||
|
||
// TODO(WB-1808, somewhatabstract): This isn't working. I got it passing | ||
// locally by calling `button.focus()` as well, but it was super flaky | ||
// and never passed first time. | ||
// Focus style | ||
// const computedStyleButton = getComputedStyle(button); | ||
// await fireEvent.focus(button); | ||
// await expect(computedStyleButton.outlineColor).toBe( | ||
// "rgb(24, 101, 242)", | ||
// ); | ||
// await expect(computedStyleButton.outlineWidth).toBe("2px"); | ||
|
||
// // Active (mouse down) style | ||
// // eslint-disable-next-line testing-library/prefer-user-event | ||
// await fireEvent.mouseDown(button); | ||
// await expect(innerLabel).toHaveStyle("color: rgb(27, 80, 179)"); | ||
// await expect(computedStyleLabel.color).toBe("rgb(27, 80, 179)"); | ||
// await expect(computedStyleLabel.height).toBe("2px"); | ||
}, | ||
}; | ||
|
||
export const styles: StyleDeclaration = StyleSheet.create({ | ||
row: { | ||
flexDirection: "row", | ||
|
@@ -203,6 +135,11 @@ Variants.parameters = { | |
story: "There are three kinds of buttons: `primary` (default), `secondary`, and `tertiary`.", | ||
}, | ||
}, | ||
chromatic: { | ||
// We already have screenshots of other stories that cover more of | ||
// the button states | ||
disableSnapshot: true, | ||
}, | ||
Comment on lines
+138
to
+142
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. note: I'm disabling a bunch of snapshots now that we can centralize visual regression tests in the |
||
}; | ||
|
||
export const WithColor: StoryComponentType = { | ||
|
@@ -324,6 +261,11 @@ Dark.parameters = { | |
story: "Buttons on a `darkBlue` background should set `light` to `true`.", | ||
}, | ||
}, | ||
chromatic: { | ||
// We already have screenshots of other stories that cover more of | ||
// the button states | ||
disableSnapshot: true, | ||
}, | ||
}; | ||
|
||
const kinds = ["primary", "secondary", "tertiary"] as const; | ||
|
@@ -539,6 +481,11 @@ Size.parameters = { | |
story: "Buttons have a size that's either `medium` (default), `small`, or `large`.", | ||
}, | ||
}, | ||
chromatic: { | ||
// We already have screenshots of other stories that cover more of | ||
// the button states | ||
disableSnapshot: true, | ||
}, | ||
}; | ||
|
||
export const Spinner: StoryComponentType = () => ( | ||
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. Would it be helpful to include the spinner state in All Variants as well? 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. I thought about that one, but I decided to keep it separately as it could introduce flakyness to that snapshot. I'm mentioning that because this variant includes animation and I've seen that Chromatic sometimes takes screenshots at different times. That said, maybe we could add it and see how it goes? wdyt? 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 point! I was thinking it could be helpful to see it across the different themes/states/variants etc, though I understand snapshots for animated things can be flaky! I'm okay leaving it as a separate story! |
||
|
@@ -789,4 +736,11 @@ export const KhanmigoTheme: StoryComponentType = { | |
</ThemeSwitcherContext.Provider> | ||
); | ||
}, | ||
parameters: { | ||
chromatic: { | ||
// We already have screenshots of other stories that cover more of | ||
// the button states | ||
disableSnapshot: true, | ||
}, | ||
}, | ||
}; |
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: Got rid of this b/c it's duplicated. The source of truth is the code in
button.tsx
.