feat(design-system): create reusable helper function to support responsive styling [AR-53842]#338
Conversation
vpolessky-dn
requested review from
StyleShit,
aroba-dn,
iromanchuk-dn and
mmurawski-dn
…n-to-support-responsive-styling
| children, | ||
| ...props | ||
| }) => { | ||
| const size = useResponsiveValue(sizeProp); |
There was a problem hiding this comment.
We rely on JS to choose the size based on the screen width. Using CSS media queries would be cheaper form performance standpoint.
I think useResponsiveValue hook is equivalent of MUI useMediaQuery and is usefull when you have some conditional rendering etc. We should expose it for library consumers.
If we want to follow MUI, then responsive props → generate CSS media queries. It may be hard, because it requires runtime css / css pre-processors. I'm not saying we 100% need it. I want to make sure we discuss this option and are on the same page.
There was a problem hiding this comment.
let discuss it on the daily
There was a problem hiding this comment.
We decided to keep it as is and do not invest time into runtime css solutions (like emotion).
|
|
||
| import breakpointTokens from '../styles/_breakpoints.module.scss'; | ||
|
|
||
| export const BREAKPOINT_LG = parseInt(breakpointTokens.breakpointLg, 10); |
There was a problem hiding this comment.
Pick question: Does it work in the build script? Just to be sure it's picked up, maybe something compiler/bundler related could be missing
|
|
||
| const getSnapshot = (): Breakpoint => (window.matchMedia(MEDIA_QUERY).matches ? 'lg' : 'md'); | ||
|
|
||
| const getServerSnapshot = (): Breakpoint => 'lg'; |
There was a problem hiding this comment.
It returns static value, why is that? Assumption server will always render the 'lg', right?
There was a problem hiding this comment.
As we do not have info during SSR about screen size it is fallback for lg size
|
|
||
| const Wrapper = (props: EnhancedProps) => { | ||
| const breakpoint = useBreakpoint(); | ||
| const resolved = { ...props } as Record<string, unknown>; |
There was a problem hiding this comment.
There is a lot of type casting. Can you verify whether it can be avoided?
There was a problem hiding this comment.
- as T (line 22) — Partial makes every lookup T | undefined; TS can't track that isResponsiveValue guarantees at least one defined value. ! would be more precise but is banned by ESLint.
- { ...props } as Record<string, unknown> (line 62) — generic EnhancedProps can't be indexed by string without widening
- Component as FunctionComponent<...> (line 72) — widened Record<string, unknown> isn't assignable back to FunctionComponent
removed cast for string
3 participants
No description provided.