Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Pull request overview
Improves the app’s small-screen/mobile experience by introducing a hamburger-driven recipe panel, alongside responsive style refinements, and adds a dev-server host setting to ease mobile testing on LAN.
Changes:
- Adds a small-screen hamburger menu / slide-in overlay for the recipe (PackingInput) panel.
- Refines responsive styling for header/footer/status bar and the small-screen warning layout.
- Updates Vite dev server configuration to bind to all network interfaces.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
vite.config.ts |
Exposes dev server on network (server.host: true) to support testing on mobile devices. |
src/App.tsx |
Implements small-screen conditional layout and hamburger menu overlay/backdrop behavior. |
src/App.css |
Adds styles for mobile menu overlay/backdrop and adjusts responsive header/footer/input sizing. |
src/components/SmallScreenWarning/style.css |
Adjusts warning overlay layout/centering for small screens. |
src/components/StatusBar/index.tsx |
Simplifies wrapper class usage for status bar layout. |
src/components/StatusBar/style.css |
Updates status bar spacing and small-screen behavior. |
package.json |
Adds @ant-design/icons dependency required by new/expanded icon usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <PackingInput startPacking={async (...args) => { | ||
| const result = startPacking(...args); | ||
| setTimeout(() => setMenuOpen(false), 400); | ||
| return result; | ||
| }} /> |
There was a problem hiding this comment.
The inline startPacking={async (...args) => ...} loses the parameter types and relies on rest/any[], which can cause noImplicitAny/strict typing issues and makes the callsite harder to understand. Consider spelling out the three parameters (or typing the rest tuple) and awaiting startPacking if you intend to react to completion.
| .mobile-menu-backdrop { | ||
| position: absolute; | ||
| inset: 0; | ||
| background: rgba(0, 0, 0, 0.4); | ||
| z-index: 499; |
There was a problem hiding this comment.
position: absolute for the mobile menu backdrop/overlay can scroll with the document and may fail to cover the viewport if the page scrolls. For an overlay menu, position: fixed is typically needed so it reliably covers the viewport and blocks background interaction.
| inset: 0; | ||
| width: 100svw; | ||
| height: 100svh; | ||
| display: flex; | ||
| align-items: center; |
There was a problem hiding this comment.
With inset: 0 this element is intended to be a full-viewport overlay, but constraining the overlay itself later (via max-width/margin) prevents it from actually filling the viewport and can break centering/click-blocking behavior. Consider keeping the container full-screen and instead constraining the inner alert.
| <Button | ||
| type="text" | ||
| icon={menuOpen ? <CloseOutlined /> : <MenuOutlined />} | ||
| onClick={() => setMenuOpen(!menuOpen)} | ||
| className="menu-toggle" |
There was a problem hiding this comment.
The icon-only menu toggle button should have an accessible name/state for screen readers (e.g., aria-label like "Open menu"/"Close menu" and aria-expanded={menuOpen}). Without this, assistive tech users won’t know what the control does.
| max-width: 320px; | ||
| margin: 0 16px; | ||
| justify-content: center; |
There was a problem hiding this comment.
max-width: 320px + margin: 0 16px on the overlay container conflicts with the full-screen overlay intent and can leave most of the screen uncovered/click-through. Prefer applying width constraints to .ant-alert (or a wrapper) and use padding on the full-screen container for side gutters.
| <h2 className="header-title">cellPACK Studio</h2> | ||
| <Button | ||
| type="text" | ||
| icon={<CloseOutlined />} |
There was a problem hiding this comment.
This icon-only close button in the mobile menu header also needs an accessible label (e.g., aria-label="Close menu").
| icon={<CloseOutlined />} | |
| icon={<CloseOutlined />} | |
| aria-label="Close menu" |
| <Button | ||
| type="text" | ||
| icon={menuOpen ? <CloseOutlined /> : <MenuOutlined />} | ||
| onClick={() => setMenuOpen(!menuOpen)} |
There was a problem hiding this comment.
Using setMenuOpen(!menuOpen) can read a stale value if React batches updates; using the functional form (setMenuOpen(prev => !prev)) avoids that class of bugs and is safer for toggle handlers.
| onClick={() => setMenuOpen(!menuOpen)} | |
| onClick={() => setMenuOpen(prev => !prev)} |
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| width: 85%; | ||
| max-width: 360px; | ||
| height: 100%; |
There was a problem hiding this comment.
Same issue here: position: absolute + height: 100% depends on the containing block height. Switching to position: fixed (with inset: 0 / top: 0) avoids cases where the menu overlay doesn’t fully cover the viewport.
| position: absolute; | |
| top: 0; | |
| left: 0; | |
| width: 85%; | |
| max-width: 360px; | |
| height: 100%; | |
| position: fixed; | |
| top: 0; | |
| left: 0; | |
| bottom: 0; | |
| width: 85%; | |
| max-width: 360px; |
| }, | ||
| plugins: [react()], | ||
| server: { | ||
| host: true, |
There was a problem hiding this comment.
server.host: true makes the Vite dev server listen on all network interfaces, which can unintentionally expose the dev build to other machines on the network. Consider gating this behind an env var/CLI flag (or documenting it) so the default remains localhost-only.
| host: true, | |
| // Bind to all network interfaces only when explicitly enabled via env var. | |
| // By default, Vite will bind to localhost for safer local development. | |
| host: | |
| process.env.VITE_BIND_ALL === 'true' || | |
| process.env.VITE_HOST_ALL === 'true' | |
| ? true | |
| : 'localhost', |
| className={`mobile-menu-backdrop ${menuOpen ? "open" : ""}`} | ||
| onClick={() => setMenuOpen(false)} |
There was a problem hiding this comment.
The backdrop is a clickable <div> but isn’t keyboard accessible. To avoid trapping keyboard-only users, consider using a semantic control (or adding role="button", tabIndex={0}, and an onKeyDown handler for Enter/Escape) so the menu can be dismissed without a pointer.
| className={`mobile-menu-backdrop ${menuOpen ? "open" : ""}`} | |
| onClick={() => setMenuOpen(false)} | |
| className={`mobile-menu-backdrop ${menuOpen ? "open" : ""}`} | |
| role="button" | |
| tabIndex={0} | |
| aria-label="Close menu" | |
| onClick={() => setMenuOpen(false)} | |
| onKeyDown={(event) => { | |
| if ( | |
| event.key === "Enter" || | |
| event.key === " " || | |
| event.key === "Escape" | |
| ) { | |
| setMenuOpen(false); | |
| } | |
| }} |
A more complete fix than #185
If the gang doesn't want to make changes late in the game, I'll just close this, but it's a much nicer look/feel.
Style tweaks all around, big change is a hamburger menu for the recipe panel.