feat(Sidebar): add resizable functionality#6340
Conversation
|
@benjamincanac ha, sorry about that, misread the silence! Reopening this. No rush on review, happy to address feedback whenever you get to it - was just trying to avoid being part of the spam/bloat train 😅 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds optional resizable behavior to Sidebar: new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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.
🧹 Nitpick comments (2)
src/runtime/components/Sidebar.vue (2)
239-241: Consider adding a clarifying comment for the sync condition.The condition
isCollapsed.value === vworks correctly becausemodelOpenandisCollapsedhave inverted semantics (when they're the same boolean value, they're actually out of sync). However, this logic is non-obvious at first glance.💡 Suggested clarification
watch(modelOpen, (v) => { + // isCollapsed and modelOpen have inverted semantics, so same values mean out-of-sync if (!isMobile.value && canCollapse.value && isCollapsed.value === v) collapse(!v) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Sidebar.vue` around lines 239 - 241, Add a short clarifying comment above the watch(...) that explains the inverted semantics: note that modelOpen and isCollapsed purposely have opposite meanings (when modelOpen.value === isCollapsed.value they are out of sync), so the check isCollapsed.value === v is intentional and collapse(!v) flips the UI to sync them; keep the existing guards (isMobile, canCollapse) and reference the watch, modelOpen, isMobile, canCollapse, isCollapsed, and collapse symbols in the comment so future readers understand the non‑obvious boolean inversion.
388-391: Event handler pattern works but has a subtle inefficiency.When
isResizableis false,handleMouseDown($event)is still evaluated (the function call is prepared even thoughundefinedis returned). While functionally correct, you could simplify with conditional binding.That said, this is a minor nitpick and the current approach is readable. No action required unless you prefer the alternative pattern.
💡 Alternative using short-circuit evaluation
- `@mousedown`="isResizable ? handleMouseDown($event) : undefined" - `@touchstart`="isResizable ? handleTouchStart($event) : undefined" - `@dblclick`="isResizable ? handleDoubleClick($event) : undefined" + `@mousedown`="isResizable && handleMouseDown($event)" + `@touchstart`="isResizable && handleTouchStart($event)" + `@dblclick`="isResizable && handleDoubleClick($event)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Sidebar.vue` around lines 388 - 391, The template currently conditionally calls handlers with ternaries (e.g., `@mousedown`="isResizable ? handleMouseDown($event) : undefined") which still evaluates the call expression; replace these with short-circuit bindings so the handler function reference is only used when isResizable is true: change `@mousedown`, `@touchstart`, and `@dblclick` to use expressions like `@mousedown`="isResizable && handleMouseDown", `@touchstart`="isResizable && handleTouchStart", `@dblclick`="isResizable && handleDoubleClick" (leave `@click`="onRailClick" as-is); this uses isResizable, handleMouseDown, handleTouchStart, and handleDoubleClick to avoid unnecessary call-site preparation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/components/Sidebar.vue`:
- Around line 239-241: Add a short clarifying comment above the watch(...) that
explains the inverted semantics: note that modelOpen and isCollapsed purposely
have opposite meanings (when modelOpen.value === isCollapsed.value they are out
of sync), so the check isCollapsed.value === v is intentional and collapse(!v)
flips the UI to sync them; keep the existing guards (isMobile, canCollapse) and
reference the watch, modelOpen, isMobile, canCollapse, isCollapsed, and collapse
symbols in the comment so future readers understand the non‑obvious boolean
inversion.
- Around line 388-391: The template currently conditionally calls handlers with
ternaries (e.g., `@mousedown`="isResizable ? handleMouseDown($event) : undefined")
which still evaluates the call expression; replace these with short-circuit
bindings so the handler function reference is only used when isResizable is
true: change `@mousedown`, `@touchstart`, and `@dblclick` to use expressions like
`@mousedown`="isResizable && handleMouseDown", `@touchstart`="isResizable &&
handleTouchStart", `@dblclick`="isResizable && handleDoubleClick" (leave
`@click`="onRailClick" as-is); this uses isResizable, handleMouseDown,
handleTouchStart, and handleDoubleClick to avoid unnecessary call-site
preparation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58d6f574-ae1b-4d1f-9690-14a462550597
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Sidebar-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Sidebar.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/app/components/content/examples/sidebar/SidebarExample.vuedocs/content/docs/2.components/dashboard-sidebar.mddocs/content/docs/2.components/sidebar.mdsrc/runtime/components/Sidebar.vuesrc/theme/sidebar.ts
…treamline event handling
commit: |
1 participant
🔗 Linked issue
Discussion in #6038
❓ Type of change
📚 Description
The new sidebar component is great, I have a similar pattern built into my own apps, but without the resizing I still couldn't use it. I've reused the composables from dashboard sidebar and tried to mimic as much of the patterns used there.
📝 Checklist