-
Notifications
You must be signed in to change notification settings - Fork 34
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
Contrast mode updates #2387
Contrast mode updates #2387
Conversation
import { ContrastMode, getBackgroundColor } from "./ThemeContrastProvider"; | ||
|
||
const useContrastMode = (explicitContrastMode?: ContrastMode) => { | ||
const ref = useRef<HTMLDivElement>(null); |
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.
In a future update, we should probably let them pass in their own wrapper. That'll be a separate discussion to have later.
shadowDomElement={shadowDomElement} | ||
shadowRootElement={shadowRootElement} | ||
themeOverride={themeOverride} | ||
contrastMode={contrastMode} |
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.
I'm concerned about exposing this contrastMode
prop. I get why it's here, but we shouldn't need it. For now, it's a fail-safe.
I'm just hoping it won't become a liability. Ideally, we'd mark it as @experimental
in TypeScript.
const updateContrastMode = useCallback(() => { | ||
if (!explicitContrastMode) { | ||
const newBgColor = getBackgroundColor(ref.current); | ||
setContrastMode( | ||
newBgColor === Tokens.HueNeutral50 ? "highContrast" : "lowContrast", | ||
); | ||
} | ||
}, [explicitContrastMode]); |
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.
We should be calling these backgroundColor
rather than bgColor
.
And in this case, I don't think we need to say newBgColor
either as that's confusing. That's the current background color.
const updateContrastMode = useCallback(() => { | |
if (!explicitContrastMode) { | |
const newBgColor = getBackgroundColor(ref.current); | |
setContrastMode( | |
newBgColor === Tokens.HueNeutral50 ? "highContrast" : "lowContrast", | |
); | |
} | |
}, [explicitContrastMode]); | |
const updateContrastMode = useCallback(() => { | |
if (!explicitContrastMode) { | |
const backgroundColor = getBackgroundColor(ref.current); | |
setContrastMode( | |
backgroundColor === Tokens.HueNeutral50 ? "highContrast" : "lowContrast", | |
); | |
} | |
}, [explicitContrastMode]); |
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.
Also, does this hook need to be here in the first place? We shouldn't be checking for Tokens.HueNeutral50
outside of ContrastModeProvider
.
Didn't we remove all this code????
useLayoutEffect(() => { | ||
const observer = new MutationObserver(updateContrastMode); | ||
observer.observe(document.querySelector("html") as HTMLHtmlElement, { | ||
attributes: true, | ||
attributeFilter: ["class", "style"], | ||
}); | ||
observer.observe(document.head, { | ||
childList: true, | ||
subtree: true, | ||
}); | ||
|
||
const onTransitionEnd = (event: TransitionEvent) => { | ||
if (event.propertyName === "background-color") { | ||
updateContrastMode(); | ||
} | ||
}; | ||
|
||
document.addEventListener("transitionend", onTransitionEnd); | ||
updateContrastMode(); | ||
|
||
return () => { | ||
document.removeEventListener("transitionend", onTransitionEnd); | ||
observer.disconnect(); | ||
}; | ||
}, [updateContrastMode]); | ||
|
||
return { ref, contrastMode }; | ||
}; |
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.
This code is duplicated in 2 places now. We should only need it in one place.
I remember it working just with the ContrastModeProvider
.
const customOdysseyTheme = useMemo( | ||
() => | ||
createTheme( | ||
deepmerge(odysseyTheme, { | ||
odysseyContrastMode: contrastMode, | ||
...(themeOverride || {}), | ||
} as ThemeOptions), | ||
), | ||
[odysseyTheme, themeOverride, contrastMode], | ||
); |
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.
This didn't have as ThemeOptions
in the old code right? I think we don't need it. Unless you're trying to say {}
is as ThemeOptions
, but that's separate from making the entire response ThemeOptions
.
I think this was added for the contrastMode
prop, but we don't need that anymore.
<div ref={ref}> | ||
<ThemeContrastContext.Provider value={contextValue}> | ||
{children} | ||
</ThemeContrastContext.Provider> | ||
</div> |
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.
This should probably be ThemeContrastModeContext
to match the naming.
export const ThemeContrastProvider: React.FC<ThemeContrastProviderProps> = ({ | ||
children, | ||
contrastMode: explicitContrastMode, | ||
}) => { |
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.
We don't need to use React.FC
here, and we don't in other places, so I'd prefer we remove it.
export const ThemeContrastProvider: React.FC<ThemeContrastProviderProps> = ({ | |
children, | |
contrastMode: explicitContrastMode, | |
}) => { | |
export const ThemeContrastProvider = ({ | |
children, | |
contrastMode: explicitContrastMode, | |
}: ThemeContrastProviderProps) => { |
contrastMode?: ContrastMode; | ||
}; | ||
|
||
export const ThemeContrastProvider: React.FC<ThemeContrastProviderProps> = ({ |
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.
I think this should probably just be named ContrastModeProvider
. It'd make a lot more sense with the prop contrastMode
, and it has nothing to do with the theme more than it has to do with the current contrast mode.
The theme provider consumes this provider and makes it relevant to the theme, but this provider only cares about setting the contrast mode.
jest.mock("../ThemeContrastProvider", () => ({ | ||
...jest.requireActual("../ThemeContrastProvider"), | ||
getBackgroundColor: jest.fn(), | ||
})); |
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.
I don't think we need to mock ThemeContrastProvider
in this test.
If we override what getBackgroundColor
does, we can't be sure it's going to work that way when apps use it.
We should wrap this test in a div
with a specific background color. That way, we can be sure it's working as-expected.
describe("ThemeContrastProvider", () => { | ||
beforeEach(() => { | ||
jest.useFakeTimers(); | ||
}); | ||
|
||
afterEach(() => { | ||
jest.useRealTimers(); | ||
jest.resetAllMocks(); | ||
}); |
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.
We shouldn't need any of these timers, but it might be because we're using requestAnimationFrame
. I wonder if we can simply wrap our expect
in act
to fix it.
const TestComponent = () => { | ||
const { contrastMode, parentBackgroundColor } = useThemeContrastContext(); | ||
return ( | ||
<div role="status" aria-label="Contrast Mode"> | ||
{contrastMode}:{parentBackgroundColor} | ||
</div> | ||
); | ||
}; |
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.
I don't know why we're creating a TestComponent
when we have real Odyssey components we can use to test with.
(getBackgroundColor as jest.Mock).mockReturnValue("#ffffff"); | ||
|
||
render( | ||
<ThemeContrastProvider> | ||
<TestComponent /> | ||
</ThemeContrastProvider>, | ||
); |
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.
This is where we can wrap ThemeContrastProvider
in a div
that includes the background color. You can even make it a styled component. We can avoid all these Jest mocks that way.
To me, jest.Mock
is generally code smell.
const { rerender } = render( | ||
<ThemeContrastProvider> | ||
<TestComponent /> | ||
</ThemeContrastProvider>, | ||
); | ||
|
||
await act(async () => { | ||
jest.runAllTimers(); | ||
}); | ||
|
||
expect( | ||
screen.getByRole("status", { name: "Contrast Mode" }), | ||
).toHaveTextContent("lowContrast:"); |
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.
I get why we have a TestComponent
now, but I think there's a better way of doing these tests. Instead of having TestComponent
with a role, we should just have it render null
.
What we want to check isn't that the HTML output is lowContrast
, we want to pass a callback onContrast
to TestComponent
, and we want to verify the value is correct.
A better way
But... There's another way of doing that same thing which doesn't require adding random test components or callback functions.
We can use ContrastModeContext.Consumer
, we can pass it a function as children
, and capture that value in a variable. Then we can say expect(contrastMode).toBe("lowContrast")
.
Now, it's type-safe, we know the expected values, and we're not adding extra code that could potentially interfere with our tests.
An even better way
It's been a while since I wrote tests against context in React.
I'm thinking you can use jest.fn
to create a function. Use that to store the value of the context.
Then you have more fine control over the current value and can say:
expect(contrastConsumerFunction).toHaveBeenCalledTimes(1)
expect(contrastConsumerFunction).toHaveBeenCalledWith({ contrastMode: "lowContrast" })
That's the best way I can think that doesn't require adding any extra code than what's built into React and Jest.
Since we're testing logic, we don't need Testing Library nor accessibility selectors.
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.
Logic testing instead
Thinking about this more, we're essentially testing a hook (all that logic) and testing the output of the context from that hook.
Testing Library has a separate library for testing React hooks. It allows you to "render" the hook and ensure, given certain values, you get a specific outcome. This way, you can test the hook outside of the context provider component, and it should mean you don't have to do such extensive component testing since the hook holds all the logic.
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.
Point is, the way we're testing this today is super hacky and isn't going to catch everything because we're mocking the getBackgroundColor
function leaving that completely untested.
And then we're adding more code that could cause us to not test correctly and receive a correct value when it's not correct.
const isTransparentColor = (color: string): boolean => | ||
color === "rgba(0, 0, 0, 0)" || color === "transparent"; |
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.
const isTransparentColor = (color: string): boolean => | |
color === "rgba(0, 0, 0, 0)" || color === "transparent"; | |
const isTransparentColor = (color: string) => | |
color === "rgba(0, 0, 0, 0)" || color === "transparent"; |
const normalizeRgbaToRgb = (rgba: string): string => | ||
rgba.replace(/rgba\((\d+), (\d+), (\d+), \d+\)/, "rgb($1, $2, $3)"); |
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.
const normalizeRgbaToRgb = (rgba: string): string => | |
rgba.replace(/rgba\((\d+), (\d+), (\d+), \d+\)/, "rgb($1, $2, $3)"); | |
const normalizeRgbaToRgb = (rgba: string) => | |
rgba.replace(/rgba\((\d+), (\d+), (\d+), \d+\)/, "rgb($1, $2, $3)"); |
|
||
const hexToRgb = (hex: string): string => { | ||
const bigint = parseInt(hex.slice(1), 16); | ||
const r = (bigint >> 16) & 255; | ||
const g = (bigint >> 8) & 255; | ||
const b = bigint & 255; | ||
return `rgb(${r}, ${g}, ${b})`; | ||
}; | ||
|
||
export const hueNeutral50Rgb = hexToRgb(Tokens.HueNeutral50); | ||
|
||
const isTransparentColor = (color: string): boolean => | ||
color === "rgba(0, 0, 0, 0)" || color === "transparent"; | ||
|
||
const normalizeRgbaToRgb = (rgba: string): string => | ||
rgba.replace(/rgba\((\d+), (\d+), (\d+), \d+\)/, "rgb($1, $2, $3)"); | ||
|
||
const getElementComputedBackgroundColor = (element: HTMLElement): string => | ||
window.getComputedStyle(element).backgroundColor; | ||
|
||
const normalizeBackgroundColor = (bgColor: string): string => { | ||
if (/rgba\((\d+), (\d+), (\d+), \d+\)/.test(bgColor)) { | ||
const normalizedColor = normalizeRgbaToRgb(bgColor); | ||
return normalizedColor === hueNeutral50Rgb | ||
? Tokens.HueNeutral50 | ||
: normalizedColor; | ||
} | ||
return bgColor === hueNeutral50Rgb ? Tokens.HueNeutral50 : bgColor; | ||
}; |
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.
We should have tests for these.
const contrastContainerRef = useRef<HTMLDivElement>(null); | ||
const { contrastMode: existingContrastMode } = useContrastModeContext(); | ||
|
||
const [parentBackgroundColor, setParentBackgroundColor] = useState("#ffffff"); |
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.
The default value of "#ffffff"
should be in an exported defaultParentBackgroundColor
variable.
That way, we can access it in our tests.
it("should update contrast mode based on background color changes", async () => { | ||
const TestComponent = () => { | ||
const { contrastContainerRef, contrastMode } = useContrastMode({}); | ||
return ( | ||
<div ref={contrastContainerRef} data-testid="container"> | ||
{contrastMode} | ||
</div> | ||
); | ||
}; | ||
|
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.
This test should probably be in 2 places. Once in OdysseyThemeProvider
, and once in getParentBackgroundColor
as this hook itself doesn't care nor does it know about getComputedStyle
being used.
it("returns the background color of the element if it is not transparent", () => { | ||
const element = document.createElement("div"); | ||
element.style.backgroundColor = "rgb(255, 0, 0)"; | ||
const result = getBackgroundColor(element); | ||
expect(result).toBe("rgb(255, 0, 0)"); | ||
}); | ||
|
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.
I like the first test to be the default case, so in this case, if it's transparent.
expect(getBackgroundColor(child)).toBe("rgb(0, 255, 0)"); | ||
}); | ||
|
||
it('returns "#ffffff" if no non-transparent background is found', () => { |
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.
it('returns "#ffffff" if no non-transparent background is found', () => { | |
it('returns "#ffffff" if transparent background is found', () => { |
const child = document.createElement("div"); | ||
grandparent.appendChild(parent); | ||
parent.appendChild(child); | ||
grandparent.style.backgroundColor = "rgb(0, 0, 255)"; |
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.
grandparent.style.backgroundColor = "rgb(0, 0, 255)"; | |
grandparent.style.setProperty("backgroundColor", "rgb(0, 0, 255)"); |
window.getComputedStyle = jest | ||
.fn() | ||
.mockImplementation((el: HTMLElement) => ({ | ||
backgroundColor: | ||
el === grandparent ? "rgb(0, 0, 255)" : "rgba(0, 0, 0, 0)", | ||
})); |
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.
window.getComputedStyle = jest | |
.fn() | |
.mockImplementation((el: HTMLElement) => ({ | |
backgroundColor: | |
el === grandparent ? "rgb(0, 0, 255)" : "rgba(0, 0, 0, 0)", | |
})); | |
window.getComputedStyle = jest | |
.fn() | |
.mockImplementation((el: HTMLElement) => (el === grandparent ? grandparent.style : { | |
backgroundColor: "rgba(0, 0, 0, 0)", | |
})); |
afterEach(() => { | ||
window.getComputedStyle = originalGetComputedStyle; | ||
jest.clearAllMocks(); | ||
document.documentElement.style.backgroundColor = ""; |
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.
I think this is the same, but if not, then put ""
instead like it was:
document.documentElement.style.backgroundColor = ""; | |
document.documentElement.style.setProperty("backgroundColor", null); |
rgba.replace(/rgba\((\d+), (\d+), (\d+), \d+\)/, "rgb($1, $2, $3)"); | ||
|
||
const getElementComputedBackgroundColor = (element: HTMLElement): string => | ||
window.getComputedStyle(element).backgroundColor; |
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.
window.getComputedStyle(element).backgroundColor; | |
window.getComputedStyle(element).getPropertyValue("backgroundColor"); |
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.
this causes the tests to fail and I think we'd have to refactor most of the tests to support it.
const event = new Event("transitionend"); | ||
Object.defineProperty(event, "propertyName", { | ||
value: "background-color", | ||
}); | ||
testContainer.dispatchEvent(event); |
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.
I think there's a better way to write this without using Object.defineProperty
. This might be it though.
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.
We should also comment this to explain why we're going through the trouble of firing the transitionend
event.
It might not be an issue with Happy-DOM; that, or we need to wrap it in a waitFor
.
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.
Maybe we can simply make the act
async.
await waitFor( | ||
() => { | ||
expect(getByTestId("container").textContent).toBe("highContrast"); | ||
}, | ||
{ timeout: 1000 }, | ||
); |
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.
I wonder if we still need this explicit timeout. No issue if we have it, just wanna make the test simpler.
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.
good call, I tested without it and it isn't needed 🎉
describe("MutationObserver functionality", () => { | ||
let originalAddEventListener: typeof document.addEventListener; | ||
let originalRemoveEventListener: typeof document.removeEventListener; | ||
|
||
beforeEach(() => { | ||
originalAddEventListener = document.addEventListener; | ||
originalRemoveEventListener = document.removeEventListener; | ||
document.addEventListener = jest.fn(); | ||
document.removeEventListener = jest.fn(); | ||
}); | ||
|
||
afterEach(() => { | ||
document.addEventListener = originalAddEventListener; | ||
document.removeEventListener = originalRemoveEventListener; | ||
}); | ||
|
||
it("should clean up observers and event listeners on unmount", () => { |
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.
This can be part of the main useContrastMode
hook tests because it's part of that hook.
What I'd like to see are the beforeEach
and afterEach
moved into the hook using the jest.spyOn
syntax.
|
||
await new Promise((resolve) => setTimeout(resolve, 500)); | ||
|
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.
This should probably be waitFor
instead.
This PR is almost done! Code's in a great state, and 99% of my comments were on minor updates to improve tests. |
…odyssey into bl_contrast_providerupdate
DES-6586
Summary
Testing & Screenshots