fix(mobile): the four step-0 shell fixes - #6765
tsahimatsliah wants to merge 7 commits into
Conversation
Step 0 of the mobile shell work (Storybook, Mobile UX / 10. The build): the fixes that need no design decision, shipped together. TabContainer's swipe decided from the total movement at touch end with a 40px threshold, so a thumb scrolling at a slight angle switched the Highlights channel. The swipe now locks its axis on the first 10px, commits past 56px inside a 27 degree cone or on a fast flick past 32px, and the surface takes touch-action: pan-y. The numbers live in shell/constants.ts. FooterNavBarLayout waited for the window load event before rendering the bar; it renders once hydrated. The footer tabs fire a click with the footer as target and the tab in extra, and Activity fires click notification icon with NotificationTarget.Footer, which existed and was never used. HighlightCardOptions and SquadOptionsButton were hidden until hover on every device, so their actions did not exist on phones; they use the laptop-and-mouse-only hover class the post cards use. The Explore period drawer gets a Close, the notification menu icon stops being rotated, Unfollow uses the remove-user icon, "Manade Ad" is "Manage ad" and the settings menu labels match their page titles. shell.css carries the shell's two curves and four durations and the press class (scale 0.96, 150ms) applied to the footer tabs, the plus button, the header controls, the feed chips and the post bar; the post titles balance. Two pre-existing strict type errors in the touched files are fixed so the strict guard passes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
rebelchris
left a comment
rebelchris
left a comment
There was a problem hiding this comment.
One should-fix (the fast-flick path in TabContainer can still switch channels during a diagonal scroll) and a few non-blocking notes inline. CI is green; the hover-only menu fix, the hydration gate replacing windowLoaded, and the label alignment look good.
Reviewed by AI.
|
|
||
| const inCone = absX > swipe.coneRatio * absY; | ||
| const farEnough = absX > swipe.commitDistance && inCone; | ||
| const fastEnough = |
There was a problem hiding this comment.
Should fix: the fast path skips the cone. The axis locks on the first 10px, so a quick diagonal scroll that happens to start a little sideways (e.g. +11x/+8y at lock, ending at 40x/90y) is locked to x, passes velocity > 0.3 && absX > 32, and changes the channel. Diagonal flicks are the common scroll gesture on a phone, so this is the original bug in another form.
Suggest requiring the cone on both paths, e.g. const fastEnough = inCone && velocity > swipe.velocity && absX > swipe.velocityDistance;, plus a spec for a fast diagonal flick that locks x and must not navigate.
Reviewed by AI.
There was a problem hiding this comment.
Agreed, and fixed in ddf9241: the cone now gates both paths (if (!inCone || (!farEnough && !fastEnough)) return), and there is a spec for exactly your gesture, a flick that locks sideways at +11/+8 and ends at 40x/90y in 100ms, which must not navigate.
| expect(onActiveClick).toHaveBeenCalledWith('Second', undefined); | ||
| }); | ||
|
|
||
| // jsdom stamps events with real time, so a drag here is always fast; the |
There was a problem hiding this comment.
Since every jsdom drag counts as fast (per this comment), the "inside the cone" case most likely passes through the velocity path, so the commitDistance path is never exercised on its own. Could the velocity be made controllable here (e.g. mocking Date.now/performance.now with the fake timers) so the distance and flick paths are each tested in isolation?
Reviewed by AI.
There was a problem hiding this comment.
Good catch, and you were right that every jsdom drag was fast. react-swipeable reads event.timeStamp, which jsdom fills with real time, so the specs now create the touch events with createEvent and stamp timeStamp themselves. The distance path is tested with a 64px drag over 600ms (0.1 px/ms), the flick path with 36px in 50ms, and the nudge stays under both.
| <div {...(swipeable ? swipeHandlers : {})}>{render}</div> | ||
| <div | ||
| {...(swipeable ? swipeHandlers : {})} | ||
| className={classNames(swipeable && 'touch-pan-y')} |
There was a problem hiding this comment.
Non-blocking (a11y): touch-action: pan-y on its own also disables pinch-zoom over the whole Highlights content. touch-pan-y touch-pinch-zoom keeps the vertical-only panning and lets users zoom.
Reviewed by AI.
There was a problem hiding this comment.
Done: touch-pan-y touch-pinch-zoom on the swipe surface.
| const logTabClick = (tab: string) => | ||
| logEvent({ | ||
| event_name: LogEvent.Click, | ||
| target_id: TargetId.MobileFooter, |
There was a problem hiding this comment.
Non-blocking (analytics): TargetId.MobileFooter is already used by the "See daily.dev in…" app footer (MobileAppFooter, impression + download app), and these generic click events set no target_type, so footer-nav taps and the app footer are hard to separate in the data. Could the nav bar get its own target (or a navigation target_type)? Since this introduces new events, worth a quick taxonomy check with Ido.
Minor: logged-out Home/Activity taps go to /onboarding but are still logged as tab clicks.
Reviewed by AI.
There was a problem hiding this comment.
Done: the tabs log TargetId.MobileFooterNav (mobile footer nav) instead of sharing MobileFooter with the app footer, and the extra carries logged_in so the taps that end at the sign-up are separable. I kept them as click events rather than dropping them; if Ido prefers a target_type, that is a one-line change. The same target will be used by the new bar in #6767.
| velocityDistance: 32, | ||
| }; | ||
|
|
||
| export const motion = { |
There was a problem hiding this comment.
Non-blocking: motion is not consumed anywhere in this PR, and it duplicates the custom properties in shell.css, which creates two sources to keep in sync. Most of the CSS vars are unused too, and .shell-press uses a plain ease-out rather than --shell-ease-interaction. Suggest adding these values with their first consumer, from a single source.
Also, per the repo's style preference, the explanatory comment blocks here, in shell.css, TabContainer.tsx and the spec would be better placed in the PR description.
Reviewed by AI.
There was a problem hiding this comment.
Done: constants.ts keeps only swipe and shell.css only .shell-press with its literal 150ms; the motion values and the custom properties arrive with their first consumers in #6767 (the block, the sheet and the bar read them there). The explanatory blocks moved into the PR description, and the remaining comments are one line each.
Chris's review: a fast diagonal flick that happened to lock sideways in its first 10px could still change the channel through the velocity path, which skipped the cone. The cone now gates both paths, with a spec for that flick; the swipe specs stamp event.timeStamp (react-swipeable's clock) so the distance and speed paths are each tested on their own. Also from the review: the swipe surface keeps pinch zoom (touch-pinch-zoom); the footer tabs log their own target (mobile footer nav) with whether the tap was a member's, so they separate from the app footer's events; the shell constants and stylesheet carry only what this PR consumes (the motion values come with their consumers in the shell PR), and the explanatory blocks moved to the PR description. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks Chris. All five addressed in ddf9241: the cone gates the fast path (with the diagonal-flick spec), the swipe specs control velocity through a stamped |
This branch was successfully deployed
2 participants
Step 0 of the mobile shell work (Storybook
Mobile UX / 10. The build, PR 0): the four fixes that need no design decision, together in one PR as agreed. Nothing moves and nothing is removed; the app is the same with four things fixed.What changed
1. The swipe that changed channels while you scrolled (Highlights)
TabContainerdecided a swipe from the total movement at touch end with a 40px threshold, so a thumb scrolling at a slight angle on a phone switched the channel (the member report from September). The axis is now decided once, on the first movement past 10px: a gesture that starts vertical is a scroll and is ignored to the end, whatever it does afterwards. A horizontal one changes the tab only when it stays inside the cone (more than twice as much sideways travel as vertical) and either travels past 56px or moves faster than 0.3px/ms over at least 32px. The cone gates both paths (review: a fast diagonal flick that locked sideways could slip through the velocity path). The swipe surface takestouch-action: pan-y pinch-zoom, so the browser never has to guess which direction a touch owns and zoom still works.2. The bottom bar at first paint, and the events it never fired
FooterNavBarLayoutwaited for the windowloadevent (images included) before rendering the bar; it now renders once hydrated. Measured on the preview the bar still mounts shortly afterload, because it arrives through two dynamic chunks after hydration; the gate on images and late scripts is gone, and true first paint (the bar in the server HTML) is PR 1.1's job. Each footer tab fires aclickwithTargetId.MobileFooteras the target and the tab inextra; the Activity tab firesclick notification iconwithNotificationTarget.Footer(an enum entry that existed and was never used) and the unread count. The bar logged nothing before. The tabs log aclickwith their own target,mobile footer nav(not the app footer'smobile footer), and{ tab, logged_in }in the extra, so a tap that ends at the sign-up is separable; Activity keeps itsclick notification iconwith the footer target and the unread count.3. Menus you could not reach on touch, and labels
HighlightCardOptionsandSquadOptionsButtonwereinvisible group-hover:visibleon every device, so their actions did not exist on phones; they use the sharedvisibleOnGroupHoverclass (laptop and mouse only) like the post cards.4. Press feedback
shared/src/styles/shell.csslands with one class,.shell-press: the control scales to 0.96 under the finger, 150ms ease-out,scalelisted beside the button colour transitions so a.btnkeeps its own, transparent tap highlight,touch-action: manipulation, and no transition under reduced motion. The footer tabs, the Create button, the post page's floating bar and the feed chips take it.shell/constants.tsholds the swipe numbers above; the motion values of the shell arrive in #6767 with the components that read them, so there is one source for them.How it was verified
TabContainer(angled scroll ignored, slow swipe past the commit distance, slow nudge ignored, fast flick, fast diagonal flick that locked sideways ignored) and aMobileFooterNavbartest for the two events.pnpm --filter shared test,pnpm --filter webapp test: green (three suites that failed under full-suite load pass alone: Spotlight, useChecklist, Feed, TagPage).scripts/typecheck-strict-changed.jsclean (including two pre-existing strict errors in the touched files, fixed); eslint clean in both packages;pnpm --filter extension buildcompiles./highlights: a scroll that starts vertical and drifts sideways past the old threshold stays on the channel and scrolls; a horizontal swipe opens the next channel; a short fast flick returns. The footer anchors computetransition-property: … scaleandtouch-action: manipulation; no hydration warnings on/.Notes for review
shell/constants.tsandshell.cssare the first files of theshellfolder the next PRs build on; each carries only what this PR consumes.createEventand stamptimeStamp, because react-swipeable reads it and jsdom would fill it with real time; the distance path and the flick path are tested apart (review).onKeyDownwould have blocked Enter now that every tab has a click handler.🤖 Generated with Claude Code
Preview domain
https://claude-mobile-shell-0-fixes.preview.app.daily.dev