-
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
fix: fixed typing for shadow root element. updated tests to reflect changes #2048
fix: fixed typing for shadow root element. updated tests to reflect changes #2048
Conversation
packages/odyssey-storybook/.storybook/components/MuiThemeDecorator.tsx
Outdated
Show resolved
Hide resolved
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.
One small comment. Otherwise looks good to me
2e85a3a
to
a9fe841
Compare
a9fe841
to
cd96f2b
Compare
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.
LGTM, selfishly happy to have this working!
const uniqueAlphabeticalId = useUniqueAlphabeticalId(); | ||
|
||
const cache = useMemo( | ||
() => | ||
createCache({ | ||
...(emotionRoot && { container: emotionRoot }), | ||
key: uniqueAlphabeticalId, | ||
prepend: true, | ||
nonce: window.cspNonce, | ||
speedy: false, | ||
}), | ||
[emotionRoot, uniqueAlphabeticalId] | ||
); | ||
|
||
if (withCache) { | ||
return ( | ||
<CacheProvider value={cache}> | ||
<MuiThemeProvider theme={customOdysseyTheme ?? odysseyTheme}> | ||
<OdysseyDesignTokensContext.Provider value={odysseyTokens}> | ||
{children} | ||
</OdysseyDesignTokensContext.Provider> | ||
</MuiThemeProvider> | ||
</CacheProvider> | ||
); | ||
} |
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 the added complexity here (if we otherwise discourage use of this provider directly). BUT if it's broken as-is (and someone is using it directly instead of OdysseyProvider), then it makes sense to make it functional.
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 agree with your thoughts on this. I don't have the context on why we need this provider (since OdysseyProvider
already includes this one), but I wanted to make sure that the emotion cache worked and was available should someone use this provider directly.
This PR removes the I want the ability to pass a custom
|
Summary
HTMLDivElement
toHTMLElement
to avoid need to castOdysseyProvider
OKTA-670823
Testing & Screenshots