-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Percipio theme + theme switcher tool #3166
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
Conversation
View your CI Pipeline Execution ↗ for commit eba1087
☁️ Nx Cloud last updated this comment at |
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.
Left some thoughts + Qs
functionality's looking great!
/** | ||
* | ||
* @param name Adds a name to the theme | ||
* This is used for referencing the theme for replacing default fonts. |
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.
Is this only for replacing fonts?
can it also be reassignment of other tokens?
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 could be! right now we're only using it for fonts, i guess it could be used for other static assets in the future 🤔
import { AssetProvider, createFontLinks } from '../AssetProvider'; | ||
import { coreTheme, percipioTheme } from '../themes'; | ||
|
||
// Using render rather than renderView because AssetProvider is used with the GamutProvider |
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.
Q: just to check — because setupRtl
uses a withMockGamutProvider
, we'd want to use render
which we can then use the actual GamutProvider? and not a mock?
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.
oh yeah i'll swap back - there was an iteration of this PR that had AssetProvider moved to within the GamutProvider but i pivoted.
font-weight: ${fontWeight.base}; | ||
return <Global styles={typographyGlobals} />; | ||
} catch (error) { | ||
// Handle font loading errors gracefully |
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.
Just a note — line 32 didn't include a comment for the console.warn
const links = container.querySelectorAll('link[rel="preload"]'); | ||
expect(links).toHaveLength(1); | ||
|
||
// Restore original fonts |
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.
Feels like robo comment
const links = container.querySelectorAll('link[rel="preload"]'); | ||
expect(links).toHaveLength(1); | ||
|
||
// Restore original fetch |
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 feels like robo comment
<GamutProvider | ||
cache={createEmotionCache({ speedy: false })} | ||
theme={storybookTheme} | ||
theme={currentTheme as any} |
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.
Is there a way to type the theme to something more narrow than any
?
If so, would that resolve some of the need to test falsey values?
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.
GamutProvider is typed to the theme.d.ts but i'll dry it up
</HelmetProvider> | ||
<SourceContainer channel={context.channel}> | ||
<ThemeProvider theme={coreTheme}> | ||
<ThemeProvider theme={currentTheme as any}> |
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.
same Q here about typing
type GlobalsContext = { | ||
globals: { | ||
colorMode: 'light' | 'dark'; | ||
theme: keyof typeof themeMap; |
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.
lol related to the typing, maybe this can serve as the base for what a type could be?
<GamutProvider | ||
useCache={false} | ||
useGlobals={false} | ||
theme={currentTheme as unknown as Theme} |
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 should've waited to comment before, but I guess it's still relevant —
could this typing work in the DocsContainer.tsx
file?
|
||
<AboutHeader {...parameters} /> | ||
|
||
The Percipio Theme is a theme used within the Percipio application. It is built off of the Core Theme with additional tokens and overrides for Percipio. For all other tokens not listed here, refer to the <LinkTo id="Foundations/Theme/CoreTheme">Core Theme</LinkTo>. |
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 link doesn't work, maybe the id
has a space in between?
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 so cool!! Still testing around onsite but initial thoughts/questions from the code
export const percipioFontFamily = { | ||
accent: '"Roboto", sans-serif', | ||
base: '"Roboto", sans-serif', | ||
monospace: '"Roboto Mono", monospace', | ||
system: '"Roboto", sans-serif', | ||
} as const; |
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.
do you think it makes sense to move this into the percipio theme file? thats what i did for lxStudio (slightly different)
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.
imo i agree unless we want to use this family outside of the percipio theme?
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 palette colors of all themes are stored in packages/gamut-styles/src/variables/colors.ts
so i thought it would make sense to follow that same logic here for typography
/* The Platform theme is a sub-theme of the Core theme, so it doesn't work as a standalone. | ||
* This is a workaround so that the Platform colors show up in the Platform theme story but keep | ||
* the main Core theme. | ||
*/ | ||
platform: storybookTheme, |
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.
how is platform theme different than the others?
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.
thought about this and figured out the issue i was having with it was only because it the only theme that was dark mode by default, fixed
// This is typed to the CoreTheme in theme.d.ts | ||
theme={currentTheme as unknown as CoreTheme} |
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.
is it being typed to CoreTheme in theme.d.ts why this is as CoreTheme
and not as Theme
like below?
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 GamutTheme looks to theme.d.ts for its theme while ThemeProvider doesn't care as long as its a valid emotion Theme. turns out i don't actually need to cast the theme below so i've removed it
.addScale('shadows', ({ colors }) => ({ | ||
sm: `0 1px 2px ${colors['emerald-200']}`, | ||
md: `0 4px 6px ${colors['emerald-300']}`, | ||
lg: `0 10px 15px ${colors['emerald-400']}`, | ||
xl: `0 20px 25px ${colors['emerald-500']}`, | ||
})) | ||
.addScale('borderRadius', () => ({ | ||
none: '0', | ||
sm: '4px', | ||
md: '8px', | ||
lg: '12px', | ||
xl: '16px', | ||
full: '9999px', | ||
})) | ||
.addScale('spacing', () => ({ | ||
xs: '4px', | ||
sm: '8px', | ||
md: '16px', | ||
lg: '24px', | ||
xl: '32px', | ||
'2xl': '48px', | ||
'3xl': '64px', | ||
})); |
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.
how does adding a scale differ from doing an override for the scale within createTheme
? is it a correct assumption that if its an existing scale you would do the override above and not add a scale? if so, can we update this to not include existing scales?
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.
great point, i clarified in my docs update!
.build(); | ||
``` | ||
|
||
## Creating a Custom Theme |
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 great!!!
accent: '"Roboto", sans-serif', | ||
base: '"Roboto", sans-serif', | ||
monospace: '"Roboto Mono", monospace', | ||
system: '"Roboto", sans-serif', |
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.
is this what is currently used? i think the way we fall back to the actual system fonts in the normal font families is better but 🤷♂️
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.
its passed to createTheme
the same way our coreTheme
does currently - i agree about the fallback and will fix!
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.
looked into this more and found this is the way its done in front right now - i'll follow up with stacey to see if we want to add more robust fallbacks in the future
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.
minor comments but overall lgtm
Co-authored-by: Amy Resnik <[email protected]>
📬Published Alpha Packages:@codecademy/[email protected] |
🚀 Styleguide deploy preview ready! |
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.
Niceeeeee! Only outstanding thing I noticed is the platform theme in dark mode doesn't look so great, but I'm fine if you tackle this as a follow up
📬 Published Alpha Packages:
|
🚀 Styleguide deploy preview ready! Preview URL: https://68efdbce446e866c84e26e83--gamut-preview.netlify.app |
Overview
Adds new Percipio theme, new font handling that allows theme specific fonts, and a Storybook theme switcher tool as well as the supporting documentation.
Static font assets are here: https://static-asset-cms.codecademy.com/browse/367
PR Checklist
Testing Instructions
mono
PR andmonolith
PRs and follow the instructions therePR Links and Envs