Enhance mobile responsiveness in Heading component#183
Conversation
- Integrated `useIsMobile` hook to conditionally manage active state and click events based on device type. - Updated state management to ensure active heading is only set on mobile devices. - Adjusted anchor visibility to show only on mobile, improving user experience across different screen sizes. This change ensures that the Heading component behaves appropriately on mobile devices, enhancing usability and performance.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new useIsMobile hook and updates the Heading component to gate active-state, anchor interactivity, and click-outside handling to mobile viewports; desktop retains navigation/scroll and hover/focus anchor behavior. Effects and handlers now depend on isMobile. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Heading
participant useIsMobile
participant DOM_Router
Note over Heading,useIsMobile: Component mount
Heading->>useIsMobile: query viewport (<=767px)
useIsMobile-->>Heading: isMobile true/false
alt isMobile = true
User->>Heading: tap heading
Heading->>Heading: set isActive (mobile)
User->>Heading: tap anchor
Heading->>Heading: set isActive (mobile)
Heading->>DOM_Router: scroll/update hash
User->>Heading: tap outside
Heading->>Heading: clear isActive
else isMobile = false
User->>Heading: click heading
Heading-->>User: no active-state change (hover/focus only)
User->>Heading: click anchor
Heading->>DOM_Router: scroll/update hash (no mobile active-state)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/www/components/ui/heading.tsx (1)
111-121: A11y: don’t focus hidden anchors on mobile; improve label.When hidden on mobile, the anchor remains in the tab order. Make it unfocusable and hidden to ATs unless active. Also, include the section id in the label.
<a href={`#${id}`} className={`select-none text-primary no-underline absolute -left-5 transition-opacity duration-200 ${ isMobile && isActive ? "opacity-100" : "opacity-0 group-hover:opacity-100 hover:opacity-100 focus:opacity-100" }`} - aria-label="Link to section" - tabIndex={0} + aria-label={`Link to section: ${id}`} + // Keep keyboard access on desktop; hide when inactive on mobile + tabIndex={isMobile && !isActive ? -1 : 0} + aria-hidden={isMobile && !isActive} onClick={handleAnchorClick} >
🧹 Nitpick comments (6)
apps/www/hooks/use-is-mobile.ts (4)
16-24: SSR‑safety, Safari fallback, and use mql.matches instead of window.innerWidth.Avoid layout reads, support older Safari, and prevent undefined window during SSR by relying on MediaQueryList.matches and feature‑detecting the listener API.
- React.useEffect(() => { - const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); - const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); - }; - mql.addEventListener("change", onChange); - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); - return () => mql.removeEventListener("change", onChange); - }, []); + React.useEffect(() => { + if (typeof window === "undefined" || !("matchMedia" in window)) { + setIsMobile(false); + return; + } + const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); + const onChange = (e: MediaQueryListEvent) => setIsMobile(e.matches); + setIsMobile(mql.matches); + if ("addEventListener" in mql) { + mql.addEventListener("change", onChange); + return () => mql.removeEventListener("change", onChange); + } + // Safari < 14 fallback + // @ts-expect-error - older Safari uses addListener/removeListener + mql.addListener(onChange); + // @ts-expect-error + return () => mql.removeListener(onChange); + }, []);
11-27: Avoid N media‑query listeners across N headings.This hook will be used by every Heading instance, creating many identical MediaQueryList subscriptions. Consider a module‑level singleton or a context/useSyncExternalStore so all consumers share one subscription.
Would you like a follow‑up patch wiring a shared store so only one matchMedia listener exists app‑wide?
3-3: Keep breakpoint in sync with design tokens.If Tailwind’s md breakpoint changes, this constant can drift. Consider importing from a shared config or referencing CSS via matchMedia on an exported token.
26-26: Nit: prefer explicit nullish coalescing.
return isMobile ?? falsereads clearer than!!isMobile.apps/www/components/ui/heading.tsx (2)
30-66: Only subscribe on mobile to cut redundant listeners; reset state on desktop.Currently every heading adds a document click listener and subscribes to active changes even on desktop. Early‑return when not mobile to reduce event overhead and avoid stale active state.
- useEffect(() => { - const handleActiveChange = (activeId: string | null) => { - // Only set active state on mobile devices - if (isMobile) { - setIsActive(activeId === id); - } - }; - - headingListeners.add(handleActiveChange); - - // Set initial state only on mobile - if (isMobile) { - setIsActive(activeHeadingId === id); - } - - const handleClickOutside = (event: MouseEvent) => { - // Only handle click outside on mobile devices - if (!isMobile) return; - - // Check if the click is outside any heading or anchor - const target = event.target as Element; - if ( - !target.closest("h1, h2, h3, h4, h5, h6") && - !target.closest('a[href^="#"]') - ) { - setActiveHeading(null); - } - }; - - document.addEventListener("click", handleClickOutside); - - return () => { - headingListeners.delete(handleActiveChange); - document.removeEventListener("click", handleClickOutside); - }; - }, [id, isMobile]); + useEffect(() => { + if (!isMobile) { + setIsActive(false); + return; + } + + const handleActiveChange = (activeId: string | null) => { + setIsActive(activeId === id); + }; + headingListeners.add(handleActiveChange); + + // initial state on mobile + setIsActive(activeHeadingId === id); + + const handleClickOutside = (event: MouseEvent) => { + const target = event.target as Element; + if ( + !target.closest("h1, h2, h3, h4, h5, h6") && + !target.closest('a[href^="#"]') + ) { + setActiveHeading(null); + } + }; + document.addEventListener("click", handleClickOutside); + + return () => { + headingListeners.delete(handleActiveChange); + document.removeEventListener("click", handleClickOutside); + }; + }, [id, isMobile]);
67-83: Preserve native anchor behaviors (new tab, copy link); avoid hash jump jitter.Allow modifier/middle‑click to pass through; use history.replaceState to avoid the second jump caused by setting location.hash after smooth scroll.
- const handleAnchorClick = (e: React.MouseEvent) => { - e.preventDefault(); + const handleAnchorClick = (e: React.MouseEvent<HTMLAnchorElement>) => { + // Allow new‑tab/copy‑link, etc. + if (e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button === 1) { + return; + } + e.preventDefault(); // Only set active state on mobile devices if (isMobile) { setActiveHeading(id ?? null); } // Always scroll to the element, even if hash is already set - const element = document.getElementById(id || ""); + const element = document.getElementById(id || ""); if (element) { element.scrollIntoView({ behavior: "smooth", block: "start" }); } - // Update the hash after scrolling - window.location.hash = `#${id}`; + // Update the hash without triggering a native jump + if (id) history.replaceState(null, "", `#${id}`); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/www/components/ui/heading.tsx(5 hunks)apps/www/hooks/use-is-mobile.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/www/components/ui/heading.tsx (2)
apps/www/hooks/use-is-mobile.ts (1)
useIsMobile(11-27)apps/www/app/docs/[[...slug]]/page.tsx (2)
props(51-51)props(47-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: test-build
🔇 Additional comments (1)
apps/www/components/ui/heading.tsx (1)
27-29: Nice: mobile‑only active state gating.Gates state churn on desktop while retaining smooth scroll/navigation.
Please sanity‑check desktop: hover/focus should still reveal anchors and keyboard users should be able to tab to them.
…ssibility - Modified the aria-label to include the section ID for better context. - Adjusted tabIndex to maintain keyboard accessibility on desktop while hiding inactive links on mobile. - Added visually hidden text for screen readers to enhance usability. These changes enhance the accessibility of the Heading component, ensuring a better experience for all users.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/www/components/ui/heading.tsx (3)
46-48: Optional: Clear on click‑outside for desktop too (symmetry).
If active is updated on desktop (e.g., via anchor click/hashchange), consider clearing it consistently.Apply this diff if you want consistent clearing:
- // Only handle click outside on mobile devices - if (!isMobile) return; + // Optionally handle on all devices for consistency + // if (!isMobile) return;
65-65: Avoid re‑registering listeners on viewport changes.
The effect doesn’t need to depend on isMobile. If you still reference isMobile inside handlers, read it from a ref.Apply this diff:
- }, [id, isMobile]); + }, [id]);And add this once near the top of the component:
// keep latest isMobile in handlers without re-subscribing const isMobileRef = useRef(isMobile); useEffect(() => { isMobileRef.current = isMobile; }, [isMobile]);Then, update the guard in the click‑outside handler:
- if (!isMobile) return; + if (!isMobileRef.current) return;
118-121: Avoid duplicate accessible names; prefer one source.
aria-label already defines the accessible name, so the sr-only text is redundant. Also, don’t force tabIndex=0 on desktop; default focusability is enough.Apply this diff:
- aria-label={`Link to section: ${id}`} - // Keep keyboard access on desktop; hide when inactive on mobile - tabIndex={isMobile && !isActive ? -1 : 0} + aria-label={`Link to section: ${id}`} + // Keep keyboard access on desktop; hide when inactive on mobile + tabIndex={isMobile && !isActive ? -1 : undefined} @@ - <span className="sr-only">Link to section: {id}</span> + {/* Accessible name provided via aria-label */} <span aria-hidden="true">#</span>Also applies to: 123-124
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/www/components/ui/heading.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/www/components/ui/heading.tsx (1)
apps/www/hooks/use-is-mobile.ts (1)
useIsMobile(11-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
apps/www/components/ui/heading.tsx (7)
5-5: Import of useIsMobile is appropriate.
No issues with the new dependency.
28-28: Hook usage is fine.
Local boolean is all you need here.
91-94: Desktop heading click behavior — confirm intent.
Not updating active on desktop clicks is fine if only anchors should drive selection there. Verify that’s the desired UX.
114-114: Anchor visibility logic looks good.
Mobile uses active; desktop keeps hover/focus.
32-35: Don’t gate active-state updates to mobile; this recreates the URL/UI mismatch.
Active heading should sync regardless of device; visibility can still be mobile-only. This also addresses the prior “Inconsistent Active State” comment.Apply this diff:
- // Only set active state on mobile devices - if (isMobile) { - setIsActive(activeId === id); - } + setIsActive(activeId === id);Additionally, wire the hash to the global active state so deep links and history nav stay in sync:
// Add once inside the component useEffect(() => { const syncFromHash = () => { const hash = window.location.hash.slice(1); setActiveHeading(hash || null); }; syncFromHash(); window.addEventListener("hashchange", syncFromHash); return () => window.removeEventListener("hashchange", syncFromHash); }, []);
70-73: Always update global active on anchor click.
Keeps global state aligned with the URL on desktop too; no visual change since desktop visibility is hover‑based.Apply this diff:
- // Only set active state on mobile devices - if (isMobile) { - setActiveHeading(id ?? null); - } + setActiveHeading(id ?? null);
40-43: Initialize from global state on all devices.
Gating the initial sync to mobile leaves stale state after resize and ignores deep links on desktop.Apply this diff:
- // Set initial state only on mobile - if (isMobile) { - setIsActive(activeHeadingId === id); - } + setIsActive(activeHeadingId === id);
useIsMobilehook to conditionally manage active state and click events based on device type.This change ensures that the Heading component behaves appropriately on mobile devices, enhancing usability and performance.
Summary by CodeRabbit
New Features
Bug Fixes