refactor(sidebar): Reuse Item component & pure css for collapsable animation (backport #6412)#6413
Conversation
|
| Filename | Overview |
|---|---|
| dashboard/src/components/navigation/sidebar/Item.vue | Refactored to a polymorphic component using NavItemProps + withDefaults; adds prefix/suffix slots and a configurable is prop. :to="route" is always bound but is safely omitted by Vue when route is undefined for non-router-link elements. |
| dashboard/src/components/navigation/sidebar/ItemGroup.vue | Replaces Vue state + CollapseTransition with native <details> + CSS grid animation using Tailwind peer selectors; the peer-open: general sibling combinator will bleed to later ItemGroup divs if a second group is ever added. |
| dashboard/src/components/navigation/sidebar/NavList.vue | Switches to v-bind spread for Item/ItemGroup props and passes only disabled to custom components; adds a -mt-1 class to the Marketplace nav entry. |
| dashboard/src/components/navigation/sidebar/Notifications.vue | Replaces custom button markup with the shared Item component; uses v-bind='$attrs' without inheritAttrs: false, causing attributes to be forwarded to both the Popover root and the Item button. |
| dashboard/src/components/navigation/sidebar/SearchItem.vue | Simplified to use the shared Item component; non-Mac shortcut label changed from Ctrl+K to Ctrl+k (lowercase). |
| dashboard/src/components/navigation/sidebar/types.ts | New file introducing the shared NavItemProps interface for typed sidebar item props. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
NL[NavList.vue] -->|v-bind item| IG[ItemGroup.vue]
NL -->|v-bind item| IT[Item.vue]
NL -->|:is customComponent| CC[Custom Component]
IG -->|is=summary| IT
IG -->|v-bind subItem| IT
CC -->|Notifications.vue v-bind attrs| IT
CC -->|SearchItem.vue| IT
IT -->|is=router-link default| RL[router-link]
IT -->|is=BUTTON| BT[button]
IT -->|is=summary| SM[summary inside details]
subgraph ItemGroup Animation
DT[details.group.peer :open=isActive]
DIV[div.peer-open:grid-rows-1fr]
DT -. CSS tilde sibling .-> DIV
end
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
dashboard/src/components/navigation/sidebar/ItemGroup.vue:22
**`peer-open:` general sibling selector bleeds across multiple groups**
Tailwind's `peer-open:` generates a CSS `~` (general sibling) selector: `.peer:open ~ .peer-open\:grid-rows-\[1fr\]`. This matches **every** subsequent sibling with the `peer-open:` class, not just the immediately adjacent one. If a second `ItemGroup` is ever added to the sidebar, opening the first group would also expand the second group's content `<div>` — because that `<div>` is a later sibling of the same `peer details:open`. Scoping with a Tailwind custom variant or wrapping the pair in a container would prevent the bleed.
### Issue 2 of 3
dashboard/src/components/navigation/sidebar/SearchItem.vue:12
The non-Mac shortcut changed from `Ctrl+K` (uppercase) to `Ctrl+k` (lowercase `k`). `Ctrl+K` is the conventional representation of this shortcut and matches the old label; lowercase `k` looks inconsistent with the Mac `⌘ K` shown alongside it.
```suggestion
{{ isMac() ? '⌘ K' : 'Ctrl+K' }}
```
### Issue 3 of 3
dashboard/src/components/navigation/sidebar/Notifications.vue:201
**`$attrs` double-forwarded without `inheritAttrs: false`**
`Notifications.vue` explicitly passes `v-bind='$attrs'` to the inner `<Item>`, but the component does not declare `inheritAttrs: false`. Because the root element is `<Popover>`, Vue 3 also automatically applies `$attrs` to `<Popover>`. Any attribute passed to `Notifications` (currently `disabled` from `NavList`) ends up forwarded to both the `<Popover>` root **and** the `<Item>` button. Adding `inheritAttrs: false` in the script block would restrict forwarding to the explicit `v-bind='$attrs'` site only.
Reviews (1): Last reviewed commit: "chore(sidebar): Avoid using $attrs" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /> | ||
| </div> | ||
| </CollapseTransition> | ||
| <div class="grid grid-rows-[0fr] transition-[grid-template-rows] duration-300 peer-open:grid-rows-[1fr]"> |
There was a problem hiding this comment.
peer-open: general sibling selector bleeds across multiple groups
Tailwind's peer-open: generates a CSS ~ (general sibling) selector: .peer:open ~ .peer-open\:grid-rows-\[1fr\]. This matches every subsequent sibling with the peer-open: class, not just the immediately adjacent one. If a second ItemGroup is ever added to the sidebar, opening the first group would also expand the second group's content <div> — because that <div> is a later sibling of the same peer details:open. Scoping with a Tailwind custom variant or wrapping the pair in a container would prevent the bleed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: dashboard/src/components/navigation/sidebar/ItemGroup.vue
Line: 22
Comment:
**`peer-open:` general sibling selector bleeds across multiple groups**
Tailwind's `peer-open:` generates a CSS `~` (general sibling) selector: `.peer:open ~ .peer-open\:grid-rows-\[1fr\]`. This matches **every** subsequent sibling with the `peer-open:` class, not just the immediately adjacent one. If a second `ItemGroup` is ever added to the sidebar, opening the first group would also expand the second group's content `<div>` — because that `<div>` is a later sibling of the same `peer details:open`. Scoping with a Tailwind custom variant or wrapping the pair in a container would prevent the bleed.
How can I resolve this? If you propose a fix, please make it concise.| <Item is='BUTTON' name='Search' :icon='LucideSearch' @click="() => (searchModalOpen = true)"> | ||
| <template #suffix> | ||
| <span class="inline-flex items-center text-sm gap-1"> | ||
| {{ isMac() ? '⌘ K' : 'Ctrl+k' }} |
There was a problem hiding this comment.
Ctrl+K (uppercase) to Ctrl+k (lowercase k). Ctrl+K is the conventional representation of this shortcut and matches the old label; lowercase k looks inconsistent with the Mac ⌘ K shown alongside it.
| {{ isMac() ? '⌘ K' : 'Ctrl+k' }} | |
| {{ isMac() ? '⌘ K' : 'Ctrl+K' }} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: dashboard/src/components/navigation/sidebar/SearchItem.vue
Line: 12
Comment:
The non-Mac shortcut changed from `Ctrl+K` (uppercase) to `Ctrl+k` (lowercase `k`). `Ctrl+K` is the conventional representation of this shortcut and matches the old label; lowercase `k` looks inconsistent with the Mac `⌘ K` shown alongside it.
```suggestion
{{ isMac() ? '⌘ K' : 'Ctrl+K' }}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| }} | ||
| </span> | ||
| </button> | ||
| <Item is='BUTTON' v-bind='$attrs' aria-label="Notifications btn" name='Notifications' @click="togglePopover"> |
There was a problem hiding this comment.
$attrs double-forwarded without inheritAttrs: false
Notifications.vue explicitly passes v-bind='$attrs' to the inner <Item>, but the component does not declare inheritAttrs: false. Because the root element is <Popover>, Vue 3 also automatically applies $attrs to <Popover>. Any attribute passed to Notifications (currently disabled from NavList) ends up forwarded to both the <Popover> root and the <Item> button. Adding inheritAttrs: false in the script block would restrict forwarding to the explicit v-bind='$attrs' site only.
Prompt To Fix With AI
This is a comment left during a code review.
Path: dashboard/src/components/navigation/sidebar/Notifications.vue
Line: 201
Comment:
**`$attrs` double-forwarded without `inheritAttrs: false`**
`Notifications.vue` explicitly passes `v-bind='$attrs'` to the inner `<Item>`, but the component does not declare `inheritAttrs: false`. Because the root element is `<Popover>`, Vue 3 also automatically applies `$attrs` to `<Popover>`. Any attribute passed to `Notifications` (currently `disabled` from `NavList`) ends up forwarded to both the `<Popover>` root **and** the `<Item>` button. Adding `inheritAttrs: false` in the script block would restrict forwarding to the explicit `v-bind='$attrs'` site only.
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6413 +/- ##
===========================================
+ Coverage 49.44% 59.71% +10.27%
===========================================
Files 941 110 -831
Lines 78180 17348 -60832
Branches 352 355 +3
===========================================
- Hits 38657 10360 -28297
+ Misses 39500 6965 -32535
Partials 23 23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🎉 This PR is included in version 0.31.5 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Remove duplicate code
Before
After
:isprop so we can make it render a link tag/button or whateverRemove JS for ItemGroup
Before
Now
<details>element to toggle content. no need of jsThis is an automatic backport of pull request #6412 done by [Mergify](https://mergify.com).