Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Pull request overview
Adds a small-screen warning banner and a few responsive CSS tweaks to improve (or at least acknowledge) usability on narrow viewports in the cellPACK Studio client.
Changes:
- Introduces a reusable
useMediaQueryhook and a newSmallScreenWarningcomponent (Ant DesignAlert) shown under 900px. - Adjusts layout sizing to use small-viewport units and adds a couple responsive CSS breakpoints for tighter spacing.
- Tunes StatusBar spacing on smaller widths.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/useMediaQuery.tsx | New hook to track media query matches for responsive UI behavior. |
| src/components/SmallScreenWarning/index.tsx | New banner component that shows a dismissible small-screen warning. |
| src/components/SmallScreenWarning/style.css | Styles the warning banner container and message wrapping. |
| src/App.tsx | Renders the warning banner at the top of the app layout. |
| src/App.css | Switches to sv* viewport units and adds basic responsive spacing/font adjustments. |
| src/components/StatusBar/style.css | Adds a 900px breakpoint to reduce gaps/padding on small screens. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| () => window.matchMedia(query).matches | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
useMediaQuery initializes state by calling window.matchMedia(query) during render. This will throw in environments where matchMedia is undefined (e.g., jsdom tests) and makes the hook harder to reuse outside the browser. Consider using optional chaining + a safe default (or deferring to an effect) similar to ThemeRoot’s window.matchMedia?.(...).matches ?? false pattern.
| () => window.matchMedia(query).matches | |
| ); | |
| useEffect(() => { | |
| () => | |
| typeof window !== "undefined" | |
| ? window.matchMedia?.(query).matches ?? false | |
| : false | |
| ); | |
| useEffect(() => { | |
| if (typeof window === "undefined" || !window.matchMedia) { | |
| return; | |
| } |
| mediaQuery.addEventListener("change", handleChange); | ||
| return () => mediaQuery.removeEventListener("change", handleChange); |
There was a problem hiding this comment.
MediaQueryList.addEventListener("change", ...) isn’t supported in some older browsers; elsewhere in the repo (ThemeRoot) this is guarded with optional chaining. Consider using addEventListener?./removeEventListener?. and/or falling back to addListener/removeListener for broader compatibility.
| mediaQuery.addEventListener("change", handleChange); | |
| return () => mediaQuery.removeEventListener("change", handleChange); | |
| const supportsAddEventListener = | |
| typeof mediaQuery.addEventListener === "function"; | |
| if (supportsAddEventListener) { | |
| mediaQuery.addEventListener("change", handleChange); | |
| } else if (typeof mediaQuery.addListener === "function") { | |
| mediaQuery.addListener(handleChange); | |
| } | |
| return () => { | |
| if (supportsAddEventListener && typeof mediaQuery.removeEventListener === "function") { | |
| mediaQuery.removeEventListener("change", handleChange); | |
| } else if (typeof mediaQuery.removeListener === "function") { | |
| mediaQuery.removeListener(handleChange); | |
| } | |
| }; |
| width: 100svw; | ||
| height: 100svh; | ||
| } | ||
|
|
||
| .header { | ||
| width: 100vw; | ||
| width: 100svw; |
There was a problem hiding this comment.
Using only svw/svh can break layout in browsers that don’t support the newer viewport units (they’ll drop the declaration entirely). Add a vw/vh fallback before the sv* declarations so unsupported browsers still get a sensible size.
| .small-screen-warning { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| width: 100svw; | ||
| z-index: 1000; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
The warning container is position: fixed at top: 0 and is rendered before the <Header>, but it doesn’t reserve space in the layout. This will visually overlay the header/content while the banner is shown. Consider using position: sticky (so it participates in flow) or adding top padding/margin equal to the banner height when the warning is visible.
Minimal fix and warning for cell pack studio site on small screens, still looks pretty busted, but we're owning it.
More extensive work in #186 if folks want to consider it.