fix: full-height sidebar with integrated space switcher#601
fix: full-height sidebar with integrated space switcher#601NagariaHussain merged 3 commits intodevelopfrom
Conversation
Move space logo and switcher dropdown from the top header into the sidebar, matching the CRM/LMS pattern. Sidebar now spans the full viewport height (h-screen, sticky top-0) instead of sitting below a 53px header bar. The entire sidebar header row is a single clickable dropdown trigger with CRM-matching Tailwind classes. The desktop header is now scoped to the content column only (search, theme toggle, nav links). Mobile layout is unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe header template was simplified: logo, space name, and the space-switcher dropdown were removed and header padding and height were adjusted. The sidebar now includes a conditional space-switcher section that appears when multiple wiki spaces exist, implemented as a fixed, scrollable dropdown controlled by Alpine state. The layout template was restructured to render a mobile-only top navbar, move the desktop header into the content column, and change outer sizing to a constant full-height flex container while keeping the sidebar alongside content. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
wiki/templates/wiki/includes/header.html (1)
3-3: Header heighth-[53px]is now a shared magic number.This
53pxheight is implicitly referenced fromwiki/templates/wiki/includes/sidebar.htmlline 39 (dropdowntop-[57px]= 53 + 4). If you tweak this header height later, the sidebar dropdown's vertical alignment will silently drift. Consider extracting to a CSS custom property (e.g.--wiki-header-height) and using it in both places, or anchoring the dropdown relative to the button with JS instead of a fixed top offset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/header.html` at line 3, The header's hardcoded h-[53px] creates a magic number shared with the sidebar dropdown's top offset (the dropdown uses top-[57px] which equals header height + 4); replace the fixed values by introducing a CSS custom property (e.g. --wiki-header-height) and use it in the header element (instead of h-[53px]) and in the sidebar dropdown positioning (compute top as calc(var(--wiki-header-height) + 4px) or equivalent), or alternatively change the dropdown to anchor relative to its toggle via JS; update the header class and the dropdown positioning logic (references: the header element with class containing h-[53px] and the dropdown that uses top-[57px]) so both derive from the same shared source.wiki/templates/wiki/includes/sidebar.html (2)
39-39: Dropdown position is fragile — hardcodedtop-[57px] left-2 w-[17rem].These three values encode implicit knowledge of the layout:
top-[57px]= sidebar-header height (h-[53px]) + 4px gap.left-2= sidebar containerpx-2.w-[17rem]= sidebarw-72(18rem) minus 2 ×left-2(1rem).Any future change to sidebar width, sidebar header height, or padding will silently misalign the dropdown without any indicator. Two better options:
- Derive the dropdown from the button with
x-ref+getBoundingClientRect()on open (handles RTL, viewport resize, and future layout changes for free).- Failing that, centralize the three constants as CSS variables (e.g.
--wiki-sidebar-w,--wiki-header-h) and consume them here.Also worth noting: because this is
position: fixed, the dropdown will not scroll with the page and will remain visible if it opens just before the user collapses the sidebar (the fixed dropdown survives the now-hidden trigger). Consider closing it on the sidebar-collapse toggle as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` at line 39, The dropdown's fixed positioning uses fragile hardcoded values (top-[57px] left-2 w-[17rem]) in the sidebar template; replace this by computing placement from the trigger button via an x-ref and calling getBoundingClientRect() on open to set inline top/left/width (handles RTL/resizes) or, if JS change is unacceptable, centralize the magic numbers into CSS variables like --wiki-sidebar-w and --wiki-header-h and consume those instead of literals; additionally ensure the dropdown is closed when the sidebar is collapsed (hook into the sidebar-collapse toggle handler to hide the dropdown) to avoid a fixed element persisting after the trigger is hidden.
17-22: Duplicated logo + space-name markup across both branches.The two branches render essentially the same logo/name block. Extracting it into a small macro (or a shared inner partial) keeps the two branches in sync if one is tweaked later.
♻️ Sketch
{% macro _space_header_content() %} {% if wiki_space.app_switcher_logo %} <img class="h-8 max-w-16 flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}" alt=""/> {% endif %} <div class="flex flex-1 flex-col text-left ml-2 truncate"> <div class="text-base font-medium leading-none text-[var(--ink-gray-9)] truncate">{{ wiki_space.space_name or _("Wiki") }}</div> </div> {% endmacro %}Then call
{{ _space_header_content() }}inside both the<button>and the simplified non-switcher<div>.Also applies to: 53-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` around lines 17 - 22, Duplicate logo/space-name markup should be extracted into a small Jinja macro (e.g., _space_header_content) that renders the conditional image (using wiki_space.app_switcher_logo) and the space title (wiki_space.space_name or _("Wiki")), then replace the duplicated blocks by calling {{ _space_header_content() }} in both places (the <button> branch and the simplified non-switcher <div>); ensure the image includes an alt attribute and keep the same classes and truncation behavior so styling remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Line 18: Add a decorative empty alt attribute to the space logo <img> elements
so screen readers skip the decorative images: update the <img> using src="{{
wiki_space.app_switcher_logo }}" (and the other space-logo <img> on line 55) to
include alt="" to satisfy WCAG and avoid reading the raw URL.
- Around line 13-28: The space-switcher button lacks accessibility attributes
and keyboard support: update the button element (the one toggling
spaceSwitcherOpen) to include type="button", add aria-haspopup="menu" and bind
:aria-expanded="spaceSwitcherOpen"; additionally implement an Escape-key handler
in the same component (where spaceSwitcherOpen is defined) that closes the menu
by setting spaceSwitcherOpen = false on Escape and removes the listener on
unmount, or attach a keydown on the button to do the same so keyboard-only users
can close the dropdown.
---
Nitpick comments:
In `@wiki/templates/wiki/includes/header.html`:
- Line 3: The header's hardcoded h-[53px] creates a magic number shared with the
sidebar dropdown's top offset (the dropdown uses top-[57px] which equals header
height + 4); replace the fixed values by introducing a CSS custom property (e.g.
--wiki-header-height) and use it in the header element (instead of h-[53px]) and
in the sidebar dropdown positioning (compute top as
calc(var(--wiki-header-height) + 4px) or equivalent), or alternatively change
the dropdown to anchor relative to its toggle via JS; update the header class
and the dropdown positioning logic (references: the header element with class
containing h-[53px] and the dropdown that uses top-[57px]) so both derive from
the same shared source.
In `@wiki/templates/wiki/includes/sidebar.html`:
- Line 39: The dropdown's fixed positioning uses fragile hardcoded values
(top-[57px] left-2 w-[17rem]) in the sidebar template; replace this by computing
placement from the trigger button via an x-ref and calling
getBoundingClientRect() on open to set inline top/left/width (handles
RTL/resizes) or, if JS change is unacceptable, centralize the magic numbers into
CSS variables like --wiki-sidebar-w and --wiki-header-h and consume those
instead of literals; additionally ensure the dropdown is closed when the sidebar
is collapsed (hook into the sidebar-collapse toggle handler to hide the
dropdown) to avoid a fixed element persisting after the trigger is hidden.
- Around line 17-22: Duplicate logo/space-name markup should be extracted into a
small Jinja macro (e.g., _space_header_content) that renders the conditional
image (using wiki_space.app_switcher_logo) and the space title
(wiki_space.space_name or _("Wiki")), then replace the duplicated blocks by
calling {{ _space_header_content() }} in both places (the <button> branch and
the simplified non-switcher <div>); ensure the image includes an alt attribute
and keep the same classes and truncation behavior so styling remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3d3a4d5-3868-4770-a569-72d236fd2b0d
📒 Files selected for processing (3)
wiki/templates/wiki/includes/header.htmlwiki/templates/wiki/includes/sidebar.htmlwiki/templates/wiki/layout.html
| :class="spaceSwitcherOpen ? 'bg-[var(--surface-white)] shadow-sm' : 'hover:bg-[var(--surface-gray-3)]'" | ||
| title="Switch wiki space"> | ||
| {% if wiki_space.app_switcher_logo %} | ||
| <img class="h-8 max-w-16 flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}"/> |
There was a problem hiding this comment.
<img> tags are missing alt attributes.
Both space-logo images lack an alt, which fails basic WCAG and also means screen readers will read the raw src URL aloud. Since the adjacent text already shows the space name, a decorative empty alt="" is the cleanest choice here.
🛠️ Proposed fix
- <img class="h-8 max-w-16 flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}"/>
+ <img class="h-8 max-w-16 flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}" alt=""/>(Apply to both occurrences, lines 18 and 55.)
Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wiki/templates/wiki/includes/sidebar.html` at line 18, Add a decorative empty
alt attribute to the space logo <img> elements so screen readers skip the
decorative images: update the <img> using src="{{ wiki_space.app_switcher_logo
}}" (and the other space-logo <img> on line 55) to include alt="" to satisfy
WCAG and avoid reading the raw URL.
Add type="button", aria-haspopup="menu", :aria-expanded, and Escape key handler to the sidebar space switcher dropdown trigger. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
wiki/templates/wiki/includes/sidebar.html (2)
10-15:⚠️ Potential issue | 🟡 MinorScope the Escape handler to the switcher component/window.
Line 15 only closes the dropdown while the trigger button has focus. After tabbing into the dropdown links, Escape will not close it. Move the handler to the Alpine component wrapper with
.windowand remove the button-scoped handler.Proposed fix
<div class="px-2 h-[53px] border-b border-[var(--outline-gray-1)] flex-shrink-0 flex items-center" - {% if has_switcher %}x-data="{ spaceSwitcherOpen: false }" `@click.outside`="spaceSwitcherOpen = false"{% endif %}> + {% if has_switcher %}x-data="{ spaceSwitcherOpen: false }" + `@click.outside`="spaceSwitcherOpen = false" + `@keydown.escape.window`="spaceSwitcherOpen = false"{% endif %}> {% if has_switcher %} <button type="button" `@click`="spaceSwitcherOpen = !spaceSwitcherOpen" - `@keydown.escape`="spaceSwitcherOpen = false" aria-haspopup="menu" :aria-expanded="spaceSwitcherOpen"Verify manually by opening the switcher, tabbing to a space link, and pressing Escape.
Alpine.js keydown escape window modifier documentation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` around lines 10 - 15, The Escape key handler is currently attached to the trigger button (`@keydown.escape`="spaceSwitcherOpen = false") so pressing Escape only works when the button has focus; instead, scope the handler to the Alpine component wrapper so it listens on the window. Move the `@keydown.escape` handler off the button and add it to the x-data wrapper (the element using has_switcher and spaceSwitcherOpen) using the window modifier (keydown.escape.window) to set spaceSwitcherOpen = false, and remove the button-scoped `@keydown.escape` from the button element.
22-22:⚠️ Potential issue | 🟡 MinorAdd decorative alt text to the space logos.
These logo images still have no
alt. Since the space name is rendered next to them, usealt=""so assistive tech skips the duplicate/decorative image.Proposed fix
-<img class="h-8 max-w-16 flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}"/> +<img class="h-8 max-w-16 flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}" alt=""/>Apply to both image occurrences.
Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wiki/templates/wiki/includes/sidebar.html` at line 22, The space logo img tags in sidebar.html are decorative and missing alt attributes; update both occurrences of the <img class="h-8 max-w-16 flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}"/> (and the other identical image at the second location) to include alt="" so assistive technologies skip the duplicate/decorative images.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wiki/templates/wiki/includes/sidebar.html`:
- Around line 10-15: The Escape key handler is currently attached to the trigger
button (`@keydown.escape`="spaceSwitcherOpen = false") so pressing Escape only
works when the button has focus; instead, scope the handler to the Alpine
component wrapper so it listens on the window. Move the `@keydown.escape` handler
off the button and add it to the x-data wrapper (the element using has_switcher
and spaceSwitcherOpen) using the window modifier (keydown.escape.window) to set
spaceSwitcherOpen = false, and remove the button-scoped `@keydown.escape` from the
button element.
- Line 22: The space logo img tags in sidebar.html are decorative and missing
alt attributes; update both occurrences of the <img class="h-8 max-w-16
flex-shrink-0" src="{{ wiki_space.app_switcher_logo }}"/> (and the other
identical image at the second location) to include alt="" so assistive
technologies skip the duplicate/decorative images.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b11b745e-0efa-49c8-825f-9f1d760247ed
📒 Files selected for processing (1)
wiki/templates/wiki/includes/sidebar.html
1 participant
Summary
h-screen,sticky top-0) instead of sitting below a 53px headerh-12,rounded-md,hover:bg-surface-gray-3, etc.)position: fixedto escape the sidebar'soverflow-hiddenclippingTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit