refactor: GP/espresso styled sidebar #468
Conversation
NagariaHussain
changed the title
|
Caution Review failedThe pull request is closed. WalkthroughAdded 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 1
🤖 Fix all issues with AI agents
In @wiki/templates/wiki/macros/sidebar_tree.html:
- Around line 6-19: The group button rendered when node.is_group needs ARIA
attributes: bind aria-expanded to the expansion state by setting
aria-expanded="{{ isExpanded('{{ node.name }}') ? 'true' : 'false' }}" (or the
framework-equivalent reactive binding) and add aria-controls referencing the id
of the group's child list (e.g., use data-node-id or a stable id like '{{
node.name }}-children'); update the toggleNode handler if necessary to ensure
the referenced child list element exists and its id matches aria-controls so
screen readers can discover the expanded/collapsed relationship.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wiki/templates/wiki/includes/mobile_header.htmlwiki/templates/wiki/includes/sidebar.htmlwiki/templates/wiki/macros/sidebar_tree.html
⏰ 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: Server
🔇 Additional comments (5)
wiki/templates/wiki/includes/sidebar.html (1)
9-9: LGTM: Horizontal padding added to navigation container.The addition of
px-2aligns the desktop sidebar navigation spacing with the mobile header navigation changes.wiki/templates/wiki/includes/mobile_header.html (1)
172-172: LGTM: Consistent horizontal padding for mobile navigation.The
px-2addition maintains consistent spacing with the desktop sidebar navigation.wiki/templates/wiki/macros/sidebar_tree.html (3)
3-3: LGTM: Enhanced container styling.The updated utility classes improve vertical spacing consistency between tree items.
33-39: LGTM: Cleaner children container layout.The switch from border-based layout to padding-based offset (
pl-4) provides clearer visual hierarchy. The expansion behavior withx-showandx-collapseis preserved correctly.
20-30: Use a template filter or validation to escape route values in JavaScript contexts.Routes can be manually set without validation and may contain single quotes, which would break the JavaScript string literals in directives like
@click.prevent="$store.navigation.navigateTo('{{ node.route }}')". Jinja2's autoescape does not escape single quotes. Either add a route validation pattern to theroutefield definition to reject quotes, or use a filter liketojsonwhen interpolating routes in JavaScript contexts.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wiki/templates/wiki/macros/sidebar_tree.html (1)
35-41: Addidattribute for ARIA reference.To complete the ARIA relationship with the group button's
aria-controls, this container needs a stable identifier.♿ Proposed fix
-<div class="wiki-children overflow-hidden pl-4 mt-0.5" +<div class="wiki-children overflow-hidden pl-4 mt-0.5" + id="{{ node.name }}-children" x-show="isExpanded('{{ node.name }}')" x-collapse>
🤖 Fix all issues with AI agents
In @wiki/templates/wiki/macros/sidebar_tree.html:
- Around line 23-32: The anchor element rendering each node (the <a> with
classes "wiki-item-content wiki-link" and attributes data-route/data-node-id)
should include aria-current="page" when the node is the current route; modify
the template to add aria-current="'page'" (or aria-current="page") conditionally
based on $store.navigation.currentRoute === '{{ node.route }}' so the attribute
appears only for the selected link and is absent for non-selected links to
improve accessibility.
- Around line 6-20: The group button for folder nodes needs ARIA attributes: add
aria-expanded="{{ isExpanded('{{ node.name }}') }}" (or dynamically bind
aria-expanded to the same boolean used by isExpanded/toggleNode) and add
aria-controls pointing to the children container ID (e.g., construct
"children-{{ node.name }}" or the ID you added to the children container), so
screen readers know the expansion state and which element is controlled; update
the button element that calls toggleNode('{{ node.name }}') to include these
attributes and ensure the children container has the matching id.
🧹 Nitpick comments (1)
wiki/templates/wiki/macros/sidebar_tree.html (1)
14-18: Consider extracting the chevron SVG.The inline SVG is repeated for every group item. While performance impact is minimal, extracting it to a shared SVG sprite or template partial would improve maintainability if the icon needs to change.
Example using SVG sprite
Define once in a shared location:
<svg style="display: none;"> <symbol id="chevron-right" viewBox="0 0 12 12"> <path d="M4 8.54V3.46c0-.4.455-.64.787-.41l3.628 2.54a.5.5 0 010 .82L4.787 8.95A.5.5 0 014 8.54z"/> </symbol> </svg>Then use it:
<svg class="w-3 h-3 mr-1.5 text-[var(--ink-gray-4)] transition-transform duration-200 flex-shrink-0" :class="isExpanded('{{ node.name }}') ? 'rotate-90' : 'rotate-0'"> <use href="#chevron-right"/> </svg>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wiki/templates/wiki/macros/sidebar_tree.html
🔇 Additional comments (2)
wiki/templates/wiki/macros/sidebar_tree.html (2)
3-3: LGTM: Clean styling foundation.The utility classes appropriately reset default list styles and establish consistent spacing for the tree structure.
31-31: Verify the conditional left padding is intentional.The
pl-[18px]for level 0 pages appears designed to align page titles with group titles (accounting for the chevron space: 12px width + 6px margin). While this achieves visual alignment, it creates a tight coupling to the chevron dimensions.Consider extracting this value to a CSS variable or design token to make the relationship explicit and easier to maintain if the chevron size changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @wiki/templates/wiki/includes/header.html:
- Around line 61-65: The search button that dispatches 'open-search' (the
<button> using render_icon("search") and the <kbd> shortcut) lacks an accessible
label; add an aria-label (e.g., aria-label="Open search" or aria-label="Open
search (Ctrl+K)") to that button element so screen readers get a descriptive
name, and ensure the label matches the visible intent of the button.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wiki/templates/wiki/includes/header.html
⏰ 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: Server
🔇 Additional comments (2)
wiki/templates/wiki/includes/header.html (2)
6-6: LGTM! Spacing refinement aligns with the redesign.The added horizontal padding and fixed width create visual alignment with the sidebar component, which is appropriate for the GP/espresso styled redesign.
10-10: LGTM! Typography refinement improves visual hierarchy.The lighter font weight and adjusted color create a softer, more refined appearance consistent with the GP/espresso design system.
| <button @click="$dispatch('open-search')" | ||
| class="inline-flex items-center gap-1 px-2 py-1 rounded-full bg-[var(--surface-gray-2)] ring-1 ring-inset ring-[var(--outline-gray-2)] hover:bg-[var(--surface-gray-3)] transition-colors duration-200 cursor-pointer"> | ||
| {{ render_icon("search", "-ml-0.5 w-4 h-4 text-[var(--ink-gray-5)]") }} | ||
| <kbd class="text-xs/4 text-[var(--ink-gray-4)] font-sans" x-text="navigator.platform.includes('Mac') ? '⌘K' : 'Ctrl + K'"></kbd> | ||
| </button> |
There was a problem hiding this comment.
Add aria-label for screen reader accessibility.
The search button lacks a text label or aria-label, making its purpose unclear to screen reader users. While the visual icon and keyboard shortcut are present, assistive technology needs an explicit label.
♿ Proposed fix to add aria-label
<button @click="$dispatch('open-search')"
+ aria-label="Search"
class="inline-flex items-center gap-1 px-2 py-1 rounded-full bg-[var(--surface-gray-2)] ring-1 ring-inset ring-[var(--outline-gray-2)] hover:bg-[var(--surface-gray-3)] transition-colors duration-200 cursor-pointer">
{{ render_icon("search", "-ml-0.5 w-4 h-4 text-[var(--ink-gray-5)]") }}
<kbd class="text-xs/4 text-[var(--ink-gray-4)] font-sans" x-text="navigator.platform.includes('Mac') ? '⌘K' : 'Ctrl + K'"></kbd>
</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button @click="$dispatch('open-search')" | |
| class="inline-flex items-center gap-1 px-2 py-1 rounded-full bg-[var(--surface-gray-2)] ring-1 ring-inset ring-[var(--outline-gray-2)] hover:bg-[var(--surface-gray-3)] transition-colors duration-200 cursor-pointer"> | |
| {{ render_icon("search", "-ml-0.5 w-4 h-4 text-[var(--ink-gray-5)]") }} | |
| <kbd class="text-xs/4 text-[var(--ink-gray-4)] font-sans" x-text="navigator.platform.includes('Mac') ? '⌘K' : 'Ctrl + K'"></kbd> | |
| </button> | |
| <button @click="$dispatch('open-search')" | |
| aria-label="Search" | |
| class="inline-flex items-center gap-1 px-2 py-1 rounded-full bg-[var(--surface-gray-2)] ring-1 ring-inset ring-[var(--outline-gray-2)] hover:bg-[var(--surface-gray-3)] transition-colors duration-200 cursor-pointer"> | |
| {{ render_icon("search", "-ml-0.5 w-4 h-4 text-[var(--ink-gray-5)]") }} | |
| <kbd class="text-xs/4 text-[var(--ink-gray-4)] font-sans" x-text="navigator.platform.includes('Mac') ? '⌘K' : 'Ctrl + K'"></kbd> | |
| </button> |
🤖 Prompt for AI Agents
In @wiki/templates/wiki/includes/header.html around lines 61 - 65, The search
button that dispatches 'open-search' (the <button> using render_icon("search")
and the <kbd> shortcut) lacks an accessible label; add an aria-label (e.g.,
aria-label="Open search" or aria-label="Open search (Ctrl+K)") to that button
element so screen readers get a descriptive name, and ensure the label matches
the visible intent of the button.
Summary by CodeRabbit
Style
Refactor
Accessibility
✏️ Tip: You can customize this high-level summary in your review settings.