feat(site): mobile bottom nav + responsive fixes#10
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…achability Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dead styles — no component or rehype plugin wraps tables in a .table-wrapper div. The existing :has(table) rule handles overflow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCloned-and-replaced interactive DOM nodes for sidebar and nav, moved click handlers and ARIA updates to the clones or live queries, adjusted mobile visibility breakpoint for bottom nav, reduced bottom padding on docs pages and added overflow-x-hidden, and changed mobile inline-code wrapping to use break-word. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NavBtn as Nav Button (cloned)
participant Sidebar as Docs Sidebar
participant Overlay as `#sidebar-overlay` (live)
User->>NavBtn: click
NavBtn->>Overlay: query current '#sidebar-overlay'
NavBtn->>Overlay: toggle hidden / set aria-hidden
NavBtn->>Sidebar: toggle open state (aria-expanded)
User->>Overlay: click
Overlay->>Sidebar: invoke closeSidebar (handler on cloned overlay)
Sidebar->>Overlay: set hidden / aria-hidden
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
site/src/components/Nav.astro (1)
78-107:⚠️ Potential issue | 🟠 MajorStale overlay reference:
initNavcaptures the old overlay node afterDocsSidebarreplaces it.
DocsSidebar.astroclones and replaces#sidebar-overlayin itsinitSidebar()function, which runs synchronously on script load. Meanwhile,initNav()is deferred viarequestAnimationFrameuntil the sidebar exists. By the timeinitNavruns and capturesoverlay(line 81), the original overlay has already been replaced byDocsSidebar, so all subsequent operations onoverlay(lines 96-97, 103-104) target a detached DOM node.🐛 Proposed fix: Re-query overlay inside the click handler
newBtn.addEventListener('click', () => { + const overlay = document.getElementById('sidebar-overlay'); + if (!overlay) return; const isOpen = !docsSidebar.classList.contains('-translate-x-full'); if (isOpen) { docsSidebar.classList.add('-translate-x-full'); docsSidebar.setAttribute('aria-hidden', 'true'); docsSidebar.inert = true; overlay.classList.add('hidden'); overlay.setAttribute('aria-hidden', 'true'); newBtn.setAttribute('aria-expanded', 'false'); } else { docsSidebar.classList.remove('-translate-x-full'); docsSidebar.setAttribute('aria-hidden', 'false'); docsSidebar.inert = false; overlay.classList.remove('hidden'); overlay.setAttribute('aria-hidden', 'false'); newBtn.setAttribute('aria-expanded', 'true'); } });And remove the early-exit check for
overlayat line 82, or keep it as a sanity check for initial load:- if (!btn || !docsSidebar || !overlay) return; + if (!btn || !docsSidebar) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/src/components/Nav.astro` around lines 78 - 107, initNav currently captures the overlay DOM node once (const overlay = document.getElementById('sidebar-overlay')) but DocsSidebar.astro's initSidebar may clone/replace that element, so the click handler operates on a stale detached node; to fix, stop relying on the cached overlay inside initNav — either remove the strict early-return for overlay or keep it as a sanity check, and re-query document.getElementById('sidebar-overlay') inside the newBtn click handler before toggling classes/aria attributes; make the same change for any overlay operations (overlay.classList..., overlay.setAttribute...) so they always target the current DOM node.site/src/components/DocsSidebar.astro (1)
190-205:⚠️ Potential issue | 🟠 MajorStale closure reference:
closeSidebaroperates on the replaced overlay node.After cloning and replacing the overlay at lines 200-201, the
closeSidebarfunction still references the originaloverlayvariable from the outer scope. WhencloseSidebaris called (via link clicks or the cloned overlay click), it will manipulate the old, removed DOM node instead ofnewOverlay.🐛 Proposed fix
+ let currentOverlay = overlay; + function closeSidebar() { mobileSidebar.classList.add('-translate-x-full'); mobileSidebar.setAttribute('aria-hidden', 'true'); mobileSidebar.inert = true; - overlay.classList.add('hidden'); - overlay.setAttribute('aria-hidden', 'true'); + currentOverlay.classList.add('hidden'); + currentOverlay.setAttribute('aria-hidden', 'true'); var menuBtn = document.getElementById('bottom-nav-docs-btn'); if (menuBtn) menuBtn.setAttribute('aria-expanded', 'false'); } const newOverlay = overlay.cloneNode(true); overlay.parentNode?.replaceChild(newOverlay, overlay); + currentOverlay = newOverlay; newOverlay.addEventListener('click', closeSidebar);Alternatively, re-query the overlay by ID inside
closeSidebarto always get the current DOM element:♻️ Alternative fix using getElementById
function closeSidebar() { + var currentOverlay = document.getElementById('sidebar-overlay'); mobileSidebar.classList.add('-translate-x-full'); mobileSidebar.setAttribute('aria-hidden', 'true'); mobileSidebar.inert = true; - overlay.classList.add('hidden'); - overlay.setAttribute('aria-hidden', 'true'); + if (currentOverlay) { + currentOverlay.classList.add('hidden'); + currentOverlay.setAttribute('aria-hidden', 'true'); + } var menuBtn = document.getElementById('bottom-nav-docs-btn'); if (menuBtn) menuBtn.setAttribute('aria-expanded', 'false'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/src/components/DocsSidebar.astro` around lines 190 - 205, closeSidebar currently closes the old replaced overlay because it references the outer-scope overlay variable; update closeSidebar to operate on the live DOM element (e.g., re-query the overlay inside the function via document.getElementById or document.querySelector) or update the outer overlay reference after cloning (assign newOverlay back to overlay) so that closeSidebar, mobileSidebar link handlers and the overlay click handler all act on the current node; target symbols: function closeSidebar, overlay.cloneNode/replaceChild, newOverlay, mobileSidebar.querySelectorAll.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site/src/styles/global.css`:
- Around line 405-407: Update the .prose code CSS rule to remove the deprecated
word-break: break-word and use the standard overflow-wrap: break-word instead;
locate the CSS selector ".prose code" and replace the word-break declaration
with overflow-wrap: break-word (optionally keep a fallback like word-break:
normal if needed for older behavior).
---
Outside diff comments:
In `@site/src/components/DocsSidebar.astro`:
- Around line 190-205: closeSidebar currently closes the old replaced overlay
because it references the outer-scope overlay variable; update closeSidebar to
operate on the live DOM element (e.g., re-query the overlay inside the function
via document.getElementById or document.querySelector) or update the outer
overlay reference after cloning (assign newOverlay back to overlay) so that
closeSidebar, mobileSidebar link handlers and the overlay click handler all act
on the current node; target symbols: function closeSidebar,
overlay.cloneNode/replaceChild, newOverlay, mobileSidebar.querySelectorAll.
In `@site/src/components/Nav.astro`:
- Around line 78-107: initNav currently captures the overlay DOM node once
(const overlay = document.getElementById('sidebar-overlay')) but
DocsSidebar.astro's initSidebar may clone/replace that element, so the click
handler operates on a stale detached node; to fix, stop relying on the cached
overlay inside initNav — either remove the strict early-return for overlay or
keep it as a sanity check, and re-query
document.getElementById('sidebar-overlay') inside the newBtn click handler
before toggling classes/aria attributes; make the same change for any overlay
operations (overlay.classList..., overlay.setAttribute...) so they always target
the current DOM node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73bbced7-670e-46a9-a282-3b912bc7c0b5
📒 Files selected for processing (5)
site/src/components/DocsSidebar.astrosite/src/components/Nav.astrosite/src/layouts/Docs.astrosite/src/pages/index.astrosite/src/styles/global.css
- DocsSidebar: move cloneNode before closeSidebar so it references the live newOverlay node, not the detached original - Nav: re-query overlay by ID inside click handler since DocsSidebar's initSidebar may clone/replace it after Nav caches the reference - CSS: replace non-standard word-break: break-word with overflow-wrap Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
site/src/components/DocsSidebar.astro (1)
93-96:⚠️ Potential issue | 🟠 MajorBreakpoint mismatch leaves mobile sidebar inaccessible on tablets.
The mobile sidebar overlay and drawer use
lg:hidden(visible until 1024px), but the bottom nav trigger button now usesmd:hidden(hidden at 768px+). This creates a gap at 768px–1024px where the mobile sidebar exists but users have no way to open it.Align the breakpoints: either change these to
md:hiddento match the bottom nav, or change the bottom nav back tolg:hidden.🔧 Proposed fix (align to md breakpoint)
<!-- Mobile sidebar overlay --> -<div id="sidebar-overlay" class="fixed inset-0 z-40 bg-black/50 hidden lg:hidden" aria-hidden="true"></div> +<div id="sidebar-overlay" class="fixed inset-0 z-40 bg-black/50 hidden md:hidden" aria-hidden="true"></div> <!-- Mobile sidebar drawer --> -<aside id="docs-sidebar-mobile" aria-hidden="true" inert class="fixed top-16 left-0 bottom-0 z-50 w-72 bg-surface-container-lowest border-r border-outline-variant transform -translate-x-full transition-transform duration-200 lg:hidden overflow-y-auto sidebar-mobile-bottom"> +<aside id="docs-sidebar-mobile" aria-hidden="true" inert class="fixed top-16 left-0 bottom-0 z-50 w-72 bg-surface-container-lowest border-r border-outline-variant transform -translate-x-full transition-transform duration-200 md:hidden overflow-y-auto sidebar-mobile-bottom">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site/src/components/DocsSidebar.astro` around lines 93 - 96, The mobile sidebar overlay (id="sidebar-overlay") and drawer (id="docs-sidebar-mobile") use the lg:hidden breakpoint while the bottom-nav trigger uses md:hidden, creating an inaccessible gap between 768–1024px; update the CSS utility classes on these elements to use md:hidden (replacing lg:hidden) so the overlay, drawer and trigger all share the same md breakpoint, or alternatively change the bottom-nav trigger back to lg:hidden—make the breakpoint consistent across sidebar-overlay, docs-sidebar-mobile, and the bottom-nav trigger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site/src/components/Nav.astro`:
- Line 38: The mobile bottom nav element with id "mobile-bottom-nav" uses the
class "md:hidden" which is inconsistent with the DocsSidebar (which uses
"lg:hidden"); update the nav's breakpoint class to match the sidebar (replace
"md:hidden" with "lg:hidden") so the mobile triggers and sidebar breakpoints are
aligned, and scan for any other mismatched breakpoint classes in the same
component to keep behavior consistent with DocsSidebar.
---
Outside diff comments:
In `@site/src/components/DocsSidebar.astro`:
- Around line 93-96: The mobile sidebar overlay (id="sidebar-overlay") and
drawer (id="docs-sidebar-mobile") use the lg:hidden breakpoint while the
bottom-nav trigger uses md:hidden, creating an inaccessible gap between
768–1024px; update the CSS utility classes on these elements to use md:hidden
(replacing lg:hidden) so the overlay, drawer and trigger all share the same md
breakpoint, or alternatively change the bottom-nav trigger back to
lg:hidden—make the breakpoint consistent across sidebar-overlay,
docs-sidebar-mobile, and the bottom-nav trigger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30353e04-3b4d-45e5-93ad-20aa30500df5
📒 Files selected for processing (3)
site/src/components/DocsSidebar.astrosite/src/components/Nav.astrosite/src/styles/global.css
The bottom nav used md:hidden while the sidebar used lg:hidden, creating a gap between 768-1024px where the nav trigger disappeared but no desktop sidebar existed. Align everything to lg. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
overflow-x-hiddenon docs container andword-break: break-wordfor inline code on mobilemdacross bottom nav visibility and padding.table-wrapperCSS (no component uses it;:has(table)handles overflow)Test plan
mdbreakpointcodeelements wrap properly on mobile🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Style