chore: navigation rework#138
Conversation
WalkthroughIntroduces a new useNavigation composable and refactors components to consume it for navigation state and routes. Updates Navigation and NavigationButton logic, removes isNavigationShown from useCatalog, adjusts several UI styles, modifies menu call behavior, and adds Russian locale keys for “Кабинет” and “Меню”. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant Scroll as WindowScroll
participant useNavigation
participant NavigationVue as Navigation.vue
participant NavBtn as NavigationButton.vue
User->>Router: Navigate (/, /client, /menu)
Router-->>useNavigation: currentRoute
Scroll-->>useNavigation: y position
useNavigation-->>NavigationVue: { isNavigationShown, isCartButtonShown, isCategoriesButtonShown, mainRoutes }
useNavigation-->>NavBtn: { isCatalogPage, canScrollToTop, isClientInnerPage, canReturnToCabinet }
User->>NavBtn: Tap button
alt isCatalogPage && canScrollToTop && isThisRoute
NavBtn->>NavBtn: handleScrollToTop()
else
NavBtn->>Router: handleRedirect(route.path)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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
🧹 Nitpick comments (11)
apps/storefront-telegram/app/components/client/BonusProgramRegistration.vue (2)
77-83: Centralize nav-visibility toggling to avoid cross-component racesDirectly mutating
isNavigationShownhere can conflict if other drawers/overlays also hide navigation. Prefer a reference-count or reason-based API inuseNavigation()(e.g.,hide('bonusDrawer')/show('bonusDrawer')) and derive visibility from active reasons.Apply locally as an interim guard:
-const { isNavigationShown } = useNavigation() +const { isNavigationShown /* , hide, show */ } = useNavigation() watch(isDrawerOpened, () => { - isNavigationShown.value = !isDrawerOpened.value + // TODO: replace with hide/show API in useNavigation to avoid clobbering + isNavigationShown.value = !isDrawerOpened.value })
55-69: Localize static Russian stringsThese user-facing strings aren’t i18n’d. Move them to locale files for consistency with the rest of navigation text.
apps/storefront-telegram/app/components/NavigationButton.vue (2)
4-4: Deduplicate the condition (readability + single source of truth)The same predicate gates click behavior and icons. Compute it once.
-@click="(isCatalogPage && canScrollToTop && isThisRoute) ? handleScrollToTop() : handleRedirect(route.path)" +@click="shouldScrollUp ? handleScrollToTop() : handleRedirect(route.path)" ... -<UIcon v-if="isCatalogPage && canScrollToTop && isThisRoute" +<UIcon v-if="shouldScrollUp" ... -<UIcon v-else-if="isClientInnerPage && canReturnToCabinet && isThisRoute" +<UIcon v-else-if="shouldShowReturnIcon"+const shouldScrollUp = computed(() => + isCatalogPage.value && canScrollToTop.value && isThisRoute.value +) +const shouldShowReturnIcon = computed(() => + isClientInnerPage.value && canReturnToCabinet.value && isThisRoute.value +)Also applies to: 13-19
2-3: Add accessible name for the buttonExpose an aria-label for SR users using route title.
-<button +<button class="flex flex-col items-center justify-center gap-1 px-4 cursor-pointer tg-text-subtitle" + :aria-label="route.title" @click="(isCatalogPage && canScrollToTop && isThisRoute) ? handleScrollToTop() : handleRedirect(route.path)" >Also applies to: 34-36
apps/storefront-telegram/app/components/Navigation.vue (1)
7-15: Button a11y: label the icon-only Categories buttonThe button is icon-only; add an aria-label (e.g., using i18n “menu”/“categories”) for SR users.
<UButton v-if="isCategoriesButtonShown" variant="outline" color="neutral" size="xl" icon="i-lucide-logs" block :ui="{ base: 'size-14 aspect-square motion-preset-slide-left motion-duration-500', }" + :aria-label="$t('menu')" />apps/storefront-telegram/app/components/cart/Button.vue (1)
10-18: Avoid hard-coded currency; format dynamicallyRender the cart total via a prop/computed and Intl currency formatting instead of a literal "1850 ₽".
- <p> - 1850 ₽ - </p> + <p>{{ totalPriceFormatted }}</p>Add outside this hunk:
<script setup lang="ts"> const props = defineProps<{ total: number }>() const totalPriceFormatted = computed(() => new Intl.NumberFormat('ru-RU', { style: 'currency', currency: 'RUB', maximumFractionDigits: 0 }).format(props.total) ) </script>apps/storefront-telegram/app/pages/client/index.vue (1)
33-58: Make “selected city” label reactive
itemsis a ref with a static array; the city label won’t update whenclientStore.selectedCitychanges. Deriveitemsfrom acomputedto react to store updates.Example outside this hunk:
const items = computed(() => [ // ... { label: clientStore.selectedCity?.name ?? 'Выбрать город', icon: 'i-lucide-locate-fixed', onClick: () => { vibrate('success'); clientStore.updateCity(null) }, }, ])apps/storefront-telegram/app/pages/menu.vue (2)
34-35: Programmatic call is fine, but consider graceful fallbackIf
tel:isn’t supported, nothing happens. Optionally wrap in a try/catch and show a toast with the number copied to clipboard.
65-69: Guard against SSR and prefer location.assignMinor: ensure it never runs server-side and use
assignfor intent clarity.-function handleCall() { - vibrate() - // Call phone number on click - window.location.href = formattedToCall -} +function handleCall() { + vibrate() + if (process.client) window.location.assign(formattedToCall) +}apps/storefront-telegram/app/composables/useNavigation.ts (2)
30-38: Prefer route.name over path literalsComparing on
route.nameavoids issues with trailing slashes and future path changes.-const isCatalogPage = computed(() => router.currentRoute.value.path === '/') +const isCatalogPage = computed(() => router.currentRoute.value.name === 'home')Similarly consider deriving “client” checks from
nameor a route meta flag instead ofstartsWith('/client').
31-31: Extract magic number for scroll thresholdName the “650” for clarity and easy tuning.
-const canScrollToTop = computed(() => y.value > 650) +const SCROLL_TO_TOP_THRESHOLD = 650 +const canScrollToTop = computed(() => y.value > SCROLL_TO_TOP_THRESHOLD)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/storefront-telegram/app/components/Navigation.vue(2 hunks)apps/storefront-telegram/app/components/NavigationButton.vue(3 hunks)apps/storefront-telegram/app/components/cart/Button.vue(1 hunks)apps/storefront-telegram/app/components/client/BonusProgramRegistration.vue(1 hunks)apps/storefront-telegram/app/components/client/PointsCard.vue(1 hunks)apps/storefront-telegram/app/composables/useCatalog.ts(0 hunks)apps/storefront-telegram/app/composables/useNavigation.ts(1 hunks)apps/storefront-telegram/app/pages/client/index.vue(1 hunks)apps/storefront-telegram/app/pages/index.vue(1 hunks)apps/storefront-telegram/app/pages/menu.vue(3 hunks)apps/storefront-telegram/app/pages/no-auth.vue(1 hunks)apps/storefront-telegram/i18n/locales/ru-RU.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/storefront-telegram/app/composables/useCatalog.ts
⏰ 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: build
🔇 Additional comments (12)
apps/storefront-telegram/app/components/client/PointsCard.vue (1)
124-124: Proper migration to centralized navigation.The change from
useCatalog()touseNavigation()for accessingisNavigationShownaligns with the PR's goal of centralizing navigation state management.apps/storefront-telegram/app/components/NavigationButton.vue (1)
45-45: Ensure reactivity survives destructuringIf
useNavigation()returns a reactive object (not plain refs), destructuring drops reactivity. Safe-guard withtoRefs.-import type { NavigationRoute } from '#shared/types/index' +import type { NavigationRoute } from '#shared/types/index' +import { toRefs } from 'vue' -const { canScrollToTop, isCatalogPage, isClientInnerPage, canReturnToCabinet } = useNavigation() +const { canScrollToTop, isCatalogPage, isClientInnerPage, canReturnToCabinet } = toRefs(useNavigation())If
useNavigation()already returns refs/computed, ignore this.apps/storefront-telegram/app/components/Navigation.vue (2)
35-40: Nice centralization via useNavigationRemoving duplicated route/visibility logic from the component and sourcing it from the composable is a solid cleanup.
2-3: Verify stickiness and safe-area after height bumpWith
sticky inset-0 h-40and innerh-16, confirm bottom positioning and no overlap with OS/Telegram UI; consider adding padding for safe area if needed.-<div v-if="clientStore.id" class="z-50 touch-pan-x sticky inset-0 h-40"> +<div v-if="clientStore.id" class="z-50 touch-pan-x sticky inset-0 h-40"> <div class="w-full h-16 px-4 py-0 flex flex-row gap-2 items-start"> ... - <nav + <nav v-if="isNavigationShown" - class="w-full h-24 tg-bg-bottom-bar border-t border-default rounded-t-lg motion-preset-slide-up" + class="w-full h-24 tg-bg-bottom-bar border-t border-default rounded-t-lg motion-preset-slide-up pb-[env(safe-area-inset-bottom)]" >apps/storefront-telegram/app/components/cart/Button.vue (1)
7-7: Padding/height tweak looks goodThe larger tap area should improve ergonomics on mobile.
apps/storefront-telegram/app/pages/client/index.vue (1)
4-4: Typography alignment LGTMHeader leading change to /6 reads better and matches other pages.
apps/storefront-telegram/app/pages/no-auth.vue (1)
3-3: Consistent header styleMatches the new heading scale. All good.
apps/storefront-telegram/i18n/locales/ru-RU.json (1)
55-56: New keys added correctly
app.cabinetandapp.menulook good and are consistent with usage.apps/storefront-telegram/app/pages/index.vue (1)
5-7: Confirm intent: static brand title replaces dynamic channel nameSwitching to a hard-coded “Суши Love” may drop per-channel branding and dynamic title updates. If multi-branding is still required, keep a dynamic fallback.
- <h1 class="text-2xl/6 font-bold tracking-tight"> - Суши Love - </h1> + <h1 class="text-2xl/6 font-bold tracking-tight">{{ brandTitle }}</h1>Add outside this hunk:
const channelStore = useChannelStore() const brandTitle = computed(() => channelStore.name || 'Суши Love') useHead({ title: brandTitle.value }) watch(brandTitle, (t) => useHead({ title: t }))apps/storefront-telegram/app/pages/menu.vue (1)
12-14: Uniform button styling via :ui is fineGood move to centralize size/leading in UI config.
apps/storefront-telegram/app/composables/useNavigation.ts (2)
1-3: Imports/types may be implicit; verify TS setupEnsure
createSharedComposable,useWindowScroll,computed,ref,useRouter, anduseI18nare resolvable (auto-imported) and thatNavigationRouteis imported or globally declared to avoid TS errors.If explicit imports are preferred:
+import { computed, ref } from 'vue' +import { useRouter, useI18n } from '#imports' +import { useWindowScroll, createSharedComposable } from '@vueuse/core' +// import type { NavigationRoute } from '~/types/navigation'
36-38: Booleans read cleanly
isCartButtonShownandisCategoriesButtonShownare straightforward and match intent.
| target="_blank" | ||
| color="neutral" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add rel to external link opened in new tab
Prevent reverse tabnabbing.
- target="_blank"
+ target="_blank"
+ rel="noopener noreferrer"📝 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.
| target="_blank" | |
| color="neutral" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| color="neutral" |
🤖 Prompt for AI Agents
In apps/storefront-telegram/app/pages/menu.vue around lines 42-43, the external
link uses target="_blank" but lacks a rel attribute which risks reverse
tabnabbing; add rel="noopener noreferrer" to the anchor element (or component
prop) that has target="_blank" so the link opens in a new tab safely, ensuring
both noopener and noreferrer are included.
1 participant
Summary by CodeRabbit