Skip to content

Fix issue where NavItem background color does not update on selected status #5873

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −10
Open

Fix issue where NavItem background color does not update on selected status #5873

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 14 additions & 10 deletions src/components/widget/nav/NavItem.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
<template>
<div
class="flex items-center gap-2 px-4 py-3 text-sm rounded-md transition-colors cursor-pointer"
:class="
active
? 'bg-white dark-theme:bg-charcoal-600 text-neutral'
: 'text-neutral hover:bg-gray-100 dark-theme:hover:bg-charcoal-300'
"
role="button"
@click="onClick"
>
<div :class="navItemClasses" role="button" @click="onClick">
<NavIcon v-if="icon" :icon="icon" />
<i-lucide:folder v-else class="text-xs text-neutral" />
<span class="flex items-center">
Expand All @@ -18,7 +9,10 @@
</template>

<script setup lang="ts">
import { computed } from 'vue'

import type { NavItemData } from '@/types/navTypes'
Copy link

Choose a reason for hiding this comment

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

[quality] low Priority

Issue: Potentially unused import
Context: The NavItemData import is only used for typing the icon prop. Consider if this import is necessary or if the type can be simplified.
Suggestion: Review if 'NavItemData["icon"]' type annotation is the most appropriate way to type the icon prop, or if a more direct type like 'string' would be clearer.

import { cn } from '@/utils/tailwindUtil'

import NavIcon from './NavIcon.vue'

Expand All @@ -27,4 +21,14 @@ const { icon, active, onClick } = defineProps<{
active?: boolean
onClick: () => void
}>()

const navItemClasses = computed(() =>
Copy link

Choose a reason for hiding this comment

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

[architecture] low Priority

Issue: Positive refactor following project conventions
Context: The refactor successfully follows the project guideline to use 'cn' utility function instead of inline class arrays, which improves code maintainability and follows established patterns.
Suggestion: Good implementation of computed property for class management. This approach is more readable and follows Vue 3 composition API best practices.

cn(
'flex items-center gap-2 px-4 py-3 text-sm rounded-md transition-colors cursor-pointer text-neutral',
{
'bg-gray-100 dark-theme:bg-charcoal-300': active,
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a little weird to me to have active toggle between standard background classes and hover modified ones.

'hover:bg-gray-100 dark-theme:hover:bg-charcoal-300': !active
Copy link

Choose a reason for hiding this comment

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

[quality] medium Priority

Issue: Hover state logic inconsistency
Context: The original implementation included hover styles for both active and inactive states, but the refactored version only applies hover when !active, potentially reducing interactivity feedback.
Suggestion: Consider if hover effects should be maintained for active items or if this change is intentional.

}
)
)
</script>