[DRAFT] Add BackgroundContext for gray background component theme changes #2362
Conversation
also removes the confusing disabled button storybook story.
| export type BackgroundType = "white" | "gray"; | ||
|
|
||
| const BackgroundContext = createContext<BackgroundType>("white"); |
There was a problem hiding this comment.
We can't use words like "white" and "gray". If we have Dark mode, they won't make sense.
That's why I was suggesting "high contrast" and "low contrast". Maybe we can figure out better words.
There was a problem hiding this comment.
Great callout. I've updated it to LowContrast (gray) / and highContrast (white) which I think will be more scalable if/when we add dark mode since it will still be relevant for styles.
| import { useBackground } from "./BackgroundContext"; // Custom hook to consume the background context | ||
| import { useButton } from "./ButtonContext"; // Custom hook to consume the button context |
There was a problem hiding this comment.
| import { useBackground } from "./BackgroundContext"; // Custom hook to consume the background context | |
| import { useButton } from "./ButtonContext"; // Custom hook to consume the button context | |
| import { useBackground } from "./BackgroundContext"; | |
| import { useButton } from "./ButtonContext"; |
| const StyledButton = styled(MuiButton, { | ||
| shouldForwardProp: (prop) => !nonForwardedProps.includes(prop as string), | ||
| })<{ |
There was a problem hiding this comment.
What's this function and why are we doing as string? I mean, I understand why relatively, but I wonder if there's a better way of doing this.
There was a problem hiding this comment.
I agree, that's a bit wonky. I updated it to be more type-safe approach:
shouldForwardProp: (prop: string) => !nonForwardedProps.includes(prop),
| const background = useBackground(); | ||
| const odysseyDesignTokens = useOdysseyDesignTokens(); | ||
| // Determine if the button is in a gray background and is a secondary variant | ||
| const isGrayBackground = background === "gray" && variantProp === "secondary"; |
There was a problem hiding this comment.
I wanna get away from gray. isLowContrastBackground or something more generic that we could use.
| const StyledButton = styled(MuiButton, { | ||
| shouldForwardProp: (prop) => !nonForwardedProps.includes(prop as string), | ||
| })<{ |
There was a problem hiding this comment.
This is a great solution to components.tsx. I wonder if we should start doing this in the future. Have a pass to localize all component styles like this.
| const canvas = within(canvasElement); | ||
| await checkButtonStyles( | ||
| canvas, | ||
| "Secondary", | ||
| "rgb(243, 244, 246)", | ||
| "rgb(143, 149, 158)", | ||
| ); |
There was a problem hiding this comment.
We can use design tokens here. I don't know what these RGB values mean otherwise or even how we got to them.
| const checkStatusStyles = async ( | ||
| canvas: ReturnType<typeof within>, | ||
| statusLabel: string, | ||
| expectedBackgroundColor: string, | ||
| expectedColor: string, | ||
| ) => { | ||
| const status = canvas.getByText(statusLabel); | ||
| await expect(status).toHaveStyle( | ||
| `background-color: ${expectedBackgroundColor}`, | ||
| ); | ||
| await expect(status).toHaveStyle(`color: ${expectedColor}`); | ||
| }; |
There was a problem hiding this comment.
Can we use Test Selectors here so we start putting that code in use?
There was a problem hiding this comment.
Sure, I updated the tests to include an ID e.g. testId: "button-medium", (I believe that's what we're after, right?)
| const checkStatusStyles = async ( | ||
| canvas: ReturnType<typeof within>, | ||
| testId: string, | ||
| expectedBackgroundColor: string, | ||
| expectedColor: string, | ||
| ) => { | ||
| const status = canvas.getByTestId(testId); | ||
| await expect(status).toHaveStyle( | ||
| `background-color: ${expectedBackgroundColor}`, | ||
| ); | ||
| await expect(status).toHaveStyle(`color: ${expectedColor}`); | ||
| }; |
There was a problem hiding this comment.
We shouldn't rely on test IDs because we're wanting to use accessibility selectors and want other people to use them as well.
We should be able to select by the "status" role. In fact, if we could, we should add that to odysseyQuerySelector. It will help consumers of Odyssey.
| ) => { | ||
| const tag = canvas.getByTestId(testId); | ||
| await expect(tag).toHaveStyle(`background-color: ${expectedBackgroundColor}`); | ||
| await expect(tag).toHaveStyle(`color: ${expectedColor}`); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Same here, we shouldn't select by test ID, we should use accessibility selectors.
Test IDs are deprecated because they're there for existing Selenium tests.
| const tag = canvasElement.querySelector(`[data-se="${args.testId}"]`); | ||
| if (tag) { | ||
| await userEvent.click(tag); | ||
| expect(args.onClick).toHaveBeenCalledTimes(1); | ||
| await axeRun("Clickable Tag"); | ||
| } else { | ||
| throw new Error(`Element with data-se="${args.testId}" not found`); | ||
| } |
There was a problem hiding this comment.
We should use testing library to select these elements.
If we use getByRole("tab", { name: "" }) instead of querySelector, it will throw an error if it can't find the element; therefore, you won't need the if (tag) {} check.
DES-6423
Summary
This is a variation of my other PR that leverages OdysseyProvider and leverages the theme to add the BackgroundProvider logic globally
isDisabledsecondaryButton,Status, andTagExample usage:
<BackgroundProvider value="lowContrast"> <Button isDisabled label="Secondary" onClick={() => {}} variant="secondary" /> </BackgroundProvider>Testing & Screenshots