Breakpoint values refactor #50
Conversation
✅ Deploy Preview for studio-design-system ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
| xxl = 1536, | ||
| } | ||
|
|
||
| // deprecated, use ScreenSize instead. | ||
| export enum Breakpoint { |
There was a problem hiding this comment.
It's definitely confusing for both of these enums to coexist. But I decided to keep the old one here for now to prevent breaking changes and let us convert in our own time.
| @@ -11,7 +11,6 @@ export enum ThemeType { | |||
| // default theme (light colors) | |||
| export const lightThemeRawValues = { | |||
| spacing, | |||
| screenSizes, | |||
There was a problem hiding this comment.
screenSizes isn't used anywhere in DS, or frontend repos, as far as I can tell. should be safe to remove
There was a problem hiding this comment.
In addition - it's better to keep ScreenSize values out of the theme IMO, since the theme is meant to contain values that may change between light/dark/other themes - but in our case, we want ScreenSize to be accessible from both VanillaExtract styling, and React hooks. Inserting themeing into the hooks would introduce unneeded complexity.
Description
This PR introduces a different enum to use for Breakpoints -
ScreenSize.Why?
Our components use a mixture of CSS media query breakpoints and React render calculations. We need to convert our breakpoint values to numbers to be used within React view calculation hooks - string value
800pxdoesn't work here. And we need these values to be consistent within.css.tsstyles and render functions.There are tons of magic numbers everywhere, and each component seems to have its own custom value for what "isMobile" means. We need a totally agnostic set of values that will work across the app. Terms like
isMobileisTabletisDesktopcan have intrinsic meaning outside of the screen width itself (eg, is the user actually on a mobile device? regardless of screen size?) - so those terms can be confusing. (Also - no clue ifMobileis bigger or smaller thanSmalland ifLargeis bigger thanDesktop?)ScreenSizenames and values were copied from TailwindCSS's standard breakpoints (https://tailwindcss.com/docs/screens#max-width-breakpoints) - which I trust to be a pretty good industry standard here.Changes Made
ScreenSizeenumuseBreakpointhooks (we previously had no way to get DS breakpoint values insiderender())Breakpoint.mdxdoc fileChecklist
I confirm that I have added relevant:
Figma (optional)
Other (optional)