refactor(web): unify mobile breakpoint into one shared isMobile store (TASK-2028)#886
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TASK-2028 — Unify the mobile breakpoint (PLAN-1984 Web-UX audit)
Two competing JS
isMobilebreakpoints (768 and 639.98) disagreed in the 640–768px band across ~15 self-rolled implementations. This replaces all of them with one shared, reactive, matchMedia-backed store.Chosen breakpoint: 768px (
<=768is mobile)Verified against the codebase rather than defaulting to the
sm(639.98) boundary:uiStore.isMobile(≤768) already drives Sidebar, TopBar, BottomNav, and MobileContextBar.@media (max-width: 768px)layout queries; the chrome CSS keys off it.{#if isMobile}→<BottomSheet>), with no matching@media (max-width: 639.98px)layout query. Aligning them up to 768 therefore introduces no CSS mismatch and avoids a sprawling CSS rewrite (which choosing 639.98 would have forced on the 20 chrome queries).WorkspaceSwitcher mobileprop) documenting the 768-vs-639.98 conflict; unifying removes the underlying disagreement.New store —
web/src/lib/stores/breakpoint.svelte.tsOne app-wide
matchMedia('change')listener writes a single$state; components only READ it (directly or viauiStore.isMobile). No$effectin the store (CONVE-1688). SSR-safe:browser-guarded, returnsfalseon the server and resolves to the real value at client module init — the same patternuiStorealready used.Call sites unified (11 files)
uiStore.isMobilenow delegates to the store; its resize listener became a singlematchMedia changelistener that only runs the sidebar/detail-panel side effects.[collection]list page, and the[collection]/[slug]item page.$effects that readviewport.isMobileand write only the local sheet-open flag (CONVE-1688 safe).isMobile && openBottomSheet gates preserved (CONVE-639);WorkspaceSwitcher'smobileprop retained (mobile-chrome slots still force the sheet). Updated the now-stale TopBar comment.Deliberately NOT changed
BottomSheet.svelte's internal@media (min-width: 640px)(centered-modal presentation on tablet widths) is left as-is. It is established behavior already relied on by the 768-gated mobile chrome (MobileContextBar → YouSheet/QuickCaptureSheet, WorkspaceSwitcher's TopBar slot), which have always mounted the sheet in the 640–768 band. Moving it to 768 would regress those consumers; leaving it makes the newly-unified consumers adopt the same coherent tablet presentation.Gates
npm run check— 0 errors (pre-existing warnings only)npm run build— successnpm run test— 153/153 passA focused unit test for the store was not feasible: the web vitest config runs in
nodewith no Svelte plugin (the suite is "plain TS, no Svelte components"), so a.svelte.tsrune module importing$app/environmentcan't be compiled there without adding Svelte+jsdom test infra (out of scope). Noted as a follow-up.