Skip to content

refactor(sidebar): Reuse Item component & pure css for collapsable animation (backport #6412) #6413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
siduck merged 2 commits into from
May 12, 2026
Merged

refactor(sidebar): Reuse Item component & pure css for collapsable animation (backport #6412) #6413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
408eddf
refactor(sidebar): reuse Item component & pure css animation for coll…
siduck May 11, 2026
c46a138
chore(sidebar): Avoid using $attrs
siduck May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 18 additions & 24 deletions dashboard/src/components/navigation/sidebar/Item.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,24 @@
<script setup lang="ts">
let props = defineProps({
item: {
type: Object,
required: true,
},
import type { NavItemProps } from "./types";

const props = withDefaults(defineProps<NavItemProps>(), {
is: "router-link",
});
</script>

<template>
<router-link
:to="item.route"
class="flex items-center rounded px-2 py-1.5 text-ink-gray-7 transition gap-2"
:class="[
item.isActive
? 'bg-surface-gray-2 md:bg-surface-white dark:bg-surface-gray-2 text-ink-gray-8 md:shadow-sm'
: 'hover:bg-surface-gray-2',
item.disabled ? 'pointer-events-none opacity-50' : '',
$attrs.class,
]"
>
<component
:is="item.icon"
class="size-4 text-ink-gray-6"
:class="item.isActive ? 'text-ink-gray-8' : ''"
/>
<span class="text-sm">{{ item.name }}</span>
<component :is="item.badge" />
</router-link>
<component :to="route" :is
class="flex text-left items-center rounded px-2 py-1.5 text-ink-gray-7 transition gap-2 w-full" :class="[
isActive
? 'bg-surface-gray-2 md:bg-surface-white dark:bg-surface-gray-2 text-ink-gray-8 md:shadow-sm'
: 'hover:bg-surface-gray-2',
disabled ? 'pointer-events-none opacity-50' : '',
]">
<slot name='prefix'>
<component :is="icon" class="size-4 text-ink-gray-6" :class="isActive ? 'text-ink-gray-8' : ''" />
</slot>

<span class="text-sm flex-1 w-full">{{ name }}</span>
<slot name="suffix" />
</component>
</template>
68 changes: 18 additions & 50 deletions dashboard/src/components/navigation/sidebar/ItemGroup.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,60 +1,28 @@
<script setup lang="ts">
import { ref, watch } from "vue";
import CollapseTransition from "@/components/utils/CollapseTransition.vue";
import LucideChevronRight from "~icons/lucide/chevron-right";
import Item from "./Item.vue";
import type { NavItemProps } from "./types";

let props = defineProps({
item: {
type: Object,
required: true,
},
});
interface Props extends NavItemProps {
children: NavItemProps[];
}

const isOpened = ref(false);

const toggle = () => {
isOpened.value = !isOpened.value;
};

watch(
() => props.item.isActive,
() => {
isOpened.value = props.item.isActive;
},
);
defineProps<Props>();
</script>

<template>
<div
@click="toggle"
class="flex cursor-pointer select-none items-center rounded px-2 py-1.5 text-ink-gray-6 transition hover:bg-surface-gray-2"
:class="[
item.disabled ? 'pointer-events-none opacity-50' : '',
$attrs.class,
]"
>
<div class="flex w-full items-center space-x-2">
<component :is="item.icon" class="size-4 text-ink-gray-6" />
<details class="group peer" :open='isActive'>
<Item is="summary" :icon :name>
<template #suffix>
<LucideChevronRight class="text-ink-gray-4 size-4 transition-transform duration-200 group-open:rotate-90" />
</template>
</Item>
</details>

<span class="text-sm">{{ item.name }}</span>
<component :is="item.badge" />
<span class="!ml-auto">
<LucideChevronRight
class="size-4 text-ink-gray-5 transition-transform duration-200"
:class="{ 'rotate-90': isOpened }"
/>
</span>
</div>
</div>
<CollapseTransition>
<div v-show="isOpened" class="ml-5 flex flex-col gap-1">
<Item
v-for="(subItem, i) in item.children"
:class="{ 'mt-0.5': i !== 0 }"
:key="subItem.name"
:item="subItem"
/>
</div>
</CollapseTransition>
<div class="grid grid-rows-[0fr] transition-[grid-template-rows] duration-300 peer-open:grid-rows-[1fr]">

@greptile-apps greptile-apps Bot May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

<div class="overflow-hidden flex flex-col gap-1 ml-5">
<Item v-for="(subItem, i) in children" :key="subItem.name" v-bind="subItem"
:class="{ 'mb-1': i == children.length - 1 }" />
</div>
</div>
</template>
7 changes: 4 additions & 3 deletions dashboard/src/components/navigation/sidebar/NavList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ const navigation = computed(() => {
icon: App,
route: "/apps",
isActive: routeName.startsWith("Marketplace"),
class: "-mt-1",
condition:
$team.doc?.is_desk_user ||
(!!$team.doc.is_developer && $session.hasAppsAccess),
Expand Down Expand Up @@ -202,8 +203,8 @@ onUnmounted(() => {

<template>
<template v-for="(item, _) in navigation" :key="item.name">
<ItemGroup v-if="item.children" :item="item" />
<component v-else-if="item.customComponent" :is="item.customComponent" :item="item" />
<Item :class="item.class" v-else :item="item" />
<ItemGroup v-if="item.children" v-bind="item" />
<component v-else-if="item.customComponent" :is="item.customComponent" :disabled="item.disabled" />
<Item v-else v-bind="item" />
</template>
</template>
50 changes: 21 additions & 29 deletions dashboard/src/components/navigation/sidebar/Notifications.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,7 @@ import { getDocResource } from "@/utils/resource";
import { renderDialog } from "@/utils/components";
import { isMobile } from "@/utils/device";

let props = defineProps({
item: {
type: Object,
required: true,
},
});
import Item from "./Item.vue";

const formatHtml = (str: string) => {
return str.replace(/<(?!\/?b\b)[^>]*>/g, "").split("\n")[0];
Expand Down Expand Up @@ -203,29 +198,26 @@ const tabs = [
<Popover :placement="isMobile() ? 'top-start' : 'right-start'" popover-class="-mt-[15%] md:-mt-2.5">
<!-- sidebar item -->
<template #target="{ togglePopover }">
<button aria-label="Notifications btn" @click="togglePopover"
class="flex items-center rounded px-2 py-1.5 text-ink-gray-6 transition gap-2 hover:bg-surface-gray-2 w-full"
:class="[
item.disabled ? 'pointer-events-none opacity-50' : '',
$attrs.class,
]">

<span class="flex relative">
<LucideBell class="size-4 text-ink-gray-6" />
<span v-if="unreadNotificationsCount.data > 0"
class="size-1 bg-surface-blue-3 rounded-full absolute right-0 -top-0.5" />
</span>

<span class="text-sm flex-1 text-left">{{ item.name }}</span>

<span class="text-xs text-ink-gray-6" v-if="unreadNotificationsCount.data > 0">
{{
unreadNotificationsCount.data > 99
? '99+'
: unreadNotificationsCount.data
}}
</span>
</button>
<Item is='BUTTON' v-bind='$attrs' aria-label="Notifications btn" name='Notifications' @click="togglePopover">

@greptile-apps greptile-apps Bot May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 $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.

<template #prefix>

<span class="flex relative">
<LucideBell class="size-4 text-ink-gray-6" />
<span v-if="unreadNotificationsCount.data > 0"
class="size-1 bg-surface-blue-3 rounded-full absolute right-0 -top-0.5" />
</span>
</template>

<template #suffix>
<span class="text-xs text-ink-gray-6" v-if="unreadNotificationsCount.data > 0">
{{
unreadNotificationsCount.data > 99
? '99+'
: unreadNotificationsCount.data
}}
</span>
</template>
</Item>
</template>

<!-- floating drawer -->
Expand Down
27 changes: 11 additions & 16 deletions dashboard/src/components/navigation/sidebar/SearchItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
<script setup lang="ts">
import LucideSearch from '~icons/lucide/search';
import { searchModalOpen } from '@/data/ui';
import { isMac } from '@/utils/device';
import { searchModalOpen } from "@/data/ui";
import { isMac } from "@/utils/device";
import LucideSearch from "~icons/lucide/search";
import Item from "./Item.vue";
</script>

<template>
<button
class="flex items-center rounded px-2 py-1.5 text-ink-gray-6 transition gap-2 text-sm w-full hover:bg-surface-gray-2"
@click="() => (searchModalOpen = true)"
>
<LucideSearch class="size-4 text-ink-gray-6" />
<span class="text-left mr-auto">Search</span>
<span v-if="isMac()" class="inline-flex items-center text-sm gap-1">
<!-- using a container to align ⌘ and K and setting font-medium to optically correct the alignment -->
<span class="text-sm font-medium">⌘</span>
<span class="text-sm">K</span>
</span>
<span v-else class="inline-flex items-center text-sm">Ctrl+K</span>
</button>
<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' }}

@greptile-apps greptile-apps Bot May 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
{{ 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>
</template>
</Item>
</template>
9 changes: 9 additions & 0 deletions dashboard/src/components/navigation/sidebar/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export interface NavItemProps {
name?: string;
icon?: any;
route?: string;
is?: string;
condition?: boolean;
disabled?: boolean;
isActive?: boolean;
}
Loading