From 106830609e5b360ec49955873a2eae6a5e62011b Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 9 Oct 2026 10:02:09 +0200 Subject: [PATCH] refactor(nav): share the context aware create action between the bottom navs The centre button resolved its target in two copies of the same hook. useCreateAction now holds it once, with the phone only branches (gallery upload, atlas search, collections) behind a flag. --- client/src/components/Layout/BottomNav.tsx | 39 +----- .../Layout/useCreateAction.test.tsx | 112 ++++++++++++++++++ .../src/components/Layout/useCreateAction.ts | 75 ++++++++++++ client/src/mobile/components/MBottomNav.tsx | 70 +---------- 4 files changed, 197 insertions(+), 99 deletions(-) create mode 100644 client/src/components/Layout/useCreateAction.test.tsx create mode 100644 client/src/components/Layout/useCreateAction.ts diff --git a/client/src/components/Layout/BottomNav.tsx b/client/src/components/Layout/BottomNav.tsx index a252dc697..bb5290c2b 100644 --- a/client/src/components/Layout/BottomNav.tsx +++ b/client/src/components/Layout/BottomNav.tsx @@ -1,4 +1,4 @@ -import { useNavigate, useLocation, useMatch } from 'react-router' +import { useNavigate, useLocation } from 'react-router' import { useAddonStore } from '../../store/addonStore' import { usePluginStore } from '../../store/pluginStore' import { useSettingsStore } from '../../store/settingsStore' @@ -8,6 +8,7 @@ import type { LucideIcon } from 'lucide-react' import { resolvePluginIcon } from '../shared/PluginIcon' import { useAuthStore } from '../../store/authStore' import { visibleManagedNavItems } from '../../managed' +import { isNavItemActive, useCreateAction } from './useCreateAction' const ADDON_NAV: Record = { vacay: { icon: CalendarDays, labelKey: 'admin.addons.catalog.vacay.name' }, @@ -18,37 +19,6 @@ const ADDON_NAV: Record = { interface NavItem { to: string; label: string; icon: LucideIcon } -// The centre "+" means something different per context: inside a trip it adds a -// place, on the journey list it starts a journey, inside a journey it adds an -// entry — everywhere else it creates a new trip. Pages pick the intent up from -// the ?create= query param. -function useCreateAction(): { label: string; run: () => void } { - const navigate = useNavigate() - const { t } = useTranslation() - const inTrip = useMatch('/trips/:id') - const inJourney = useMatch('/journey/:id') - const onJourneyList = useMatch('/journey') - - if (inTrip) { - // The "+" is context-aware per active tab: Bookings → reservation, - // Transports → transport, Costs → expense. Tabs without a create modal - // (lists / files / collab) fall through to adding a place. #1349 - const id = inTrip.params.id - const tripTab = typeof sessionStorage !== 'undefined' ? sessionStorage.getItem(`trip-tab-${id}`) : null - if (tripTab === 'finanzplan') return { label: t('costs.addExpense'), run: () => navigate(`/trips/${id}?create=expense`) } - if (tripTab === 'buchungen') return { label: t('reservations.addManual'), run: () => navigate(`/trips/${id}?create=reservation`) } - if (tripTab === 'transports') return { label: t('transport.addManual'), run: () => navigate(`/trips/${id}?create=transport`) } - return { label: t('places.addPlace'), run: () => navigate(`/trips/${id}?create=place`) } - } - if (inJourney) { - return { label: t('journey.detail.addEntry'), run: () => navigate(`/journey/${inJourney.params.id}?create=entry`) } - } - if (onJourneyList) { - return { label: t('journey.new'), run: () => navigate('/journey?create=1') } - } - return { label: t('dashboard.newTrip'), run: () => navigate('/dashboard?create=1') } -} - export default function BottomNav() { const { t } = useTranslation() const navigate = useNavigate() @@ -60,7 +30,7 @@ export default function BottomNav() { // nav pill (Navbar) — otherwise they were only reachable by typing /plugins/:id. const pagePlugins = usePluginStore(s => s.plugins).filter(p => p.type === 'page') const location = useLocation() - const create = useCreateAction() + const create = useCreateAction(false) const isAdmin = useAuthStore(s => s.user?.role === 'admin') const items: NavItem[] = [ @@ -78,8 +48,7 @@ export default function BottomNav() { const left = items.slice(0, splitAt) const right = items.slice(splitAt) - const isActive = (to: string) => - to === '/dashboard' ? location.pathname === '/dashboard' : location.pathname.startsWith(to) + const isActive = (to: string) => isNavItemActive(location.pathname, to) const renderItem = ({ to, label, icon: Icon }: NavItem) => { const active = isActive(to) diff --git a/client/src/components/Layout/useCreateAction.test.tsx b/client/src/components/Layout/useCreateAction.test.tsx new file mode 100644 index 000000000..fa5f9066b --- /dev/null +++ b/client/src/components/Layout/useCreateAction.test.tsx @@ -0,0 +1,112 @@ +// FE-COMP-CREATEACTION-001 to -009: the context-aware "+" both bottom navs share. +import { act, renderHook } from '@testing-library/react'; +import type { ReactNode } from 'react'; +import { MemoryRouter } from 'react-router'; + +import { useJourneyStore } from '../../store/journeyStore'; +import { isNavItemActive, useCreateAction } from './useCreateAction'; + +const mockNavigate = vi.fn(); +vi.mock('react-router', async () => { + const actual = await vi.importActual('react-router'); + return { ...actual, useNavigate: () => mockNavigate }; +}); +vi.mock('../../i18n', () => ({ useTranslation: () => ({ t: (k: string) => k }) })); + +function at(path: string, phone: boolean) { + const wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + return renderHook(() => useCreateAction(phone), { wrapper }).result.current; +} + +beforeEach(() => { + mockNavigate.mockClear(); + sessionStorage.clear(); + useJourneyStore.setState({ mobileGalleryOpen: false }); +}); + +describe('useCreateAction', () => { + it('FE-COMP-CREATEACTION-001: creates a trip by default on both navs', () => { + for (const phone of [false, true]) { + const action = at('/dashboard', phone); + expect(action.label).toBe('dashboard.newTrip'); + act(() => action.run()); + expect(mockNavigate).toHaveBeenLastCalledWith('/dashboard?create=1'); + } + }); + + it('FE-COMP-CREATEACTION-002: inside a trip it follows the remembered tab', () => { + const cases: [string | null, string, string][] = [ + ['finanzplan', 'costs.addExpense', 'expense'], + ['buchungen', 'reservations.addManual', 'reservation'], + ['transports', 'transport.addManual', 'transport'], + ['lists', 'places.addPlace', 'place'], + [null, 'places.addPlace', 'place'], + ]; + for (const [tab, label, create] of cases) { + sessionStorage.clear(); + if (tab) sessionStorage.setItem('trip-tab-7', tab); + const action = at('/trips/7', false); + expect(action.label).toBe(label); + act(() => action.run()); + expect(mockNavigate).toHaveBeenLastCalledWith(`/trips/7?create=${create}`); + } + }); + + it('FE-COMP-CREATEACTION-003: inside a journey it adds an entry', () => { + const action = at('/journey/3', true); + expect(action).toMatchObject({ label: 'journey.detail.addEntry' }); + expect(action.upload).toBeUndefined(); + act(() => action.run()); + expect(mockNavigate).toHaveBeenLastCalledWith('/journey/3?create=entry'); + }); + + it('FE-COMP-CREATEACTION-004: the phone uploads a photo while the journey gallery is open', () => { + useJourneyStore.setState({ mobileGalleryOpen: true }); + const action = at('/journey/3', true); + expect(action).toMatchObject({ label: 'common.upload', upload: true }); + act(() => action.run()); + expect(mockNavigate).toHaveBeenLastCalledWith('/journey/3?create=photo'); + }); + + it('FE-COMP-CREATEACTION-005: the desktop nav ignores the open gallery', () => { + useJourneyStore.setState({ mobileGalleryOpen: true }); + expect(at('/journey/3', false).label).toBe('journey.detail.addEntry'); + }); + + it('FE-COMP-CREATEACTION-006: the journey list starts a journey', () => { + const action = at('/journey', false); + expect(action.label).toBe('journey.new'); + act(() => action.run()); + expect(mockNavigate).toHaveBeenLastCalledWith('/journey?create=1'); + }); + + it('FE-COMP-CREATEACTION-007: the atlas opens the country search on the phone only', () => { + const phone = at('/atlas', true); + expect(phone.label).toBe('atlas.searchCountry'); + act(() => phone.run()); + expect(mockNavigate).toHaveBeenLastCalledWith('/atlas?search=1'); + expect(at('/atlas', false).label).toBe('dashboard.newTrip'); + }); + + it('FE-COMP-CREATEACTION-008: collections add a place to the open list on the phone only', () => { + const all = at('/collections', true); + expect(all.label).toBe('collections.addPlace'); + act(() => all.run()); + expect(mockNavigate).toHaveBeenLastCalledWith('/collections?create=place'); + const one = at('/collections/12', true); + act(() => one.run()); + expect(mockNavigate).toHaveBeenLastCalledWith('/collections/12?create=place'); + expect(at('/collections/12', false).label).toBe('dashboard.newTrip'); + }); +}); + +describe('isNavItemActive', () => { + it('FE-COMP-CREATEACTION-009: the dashboard matches exactly, other items by prefix', () => { + expect(isNavItemActive('/dashboard', '/dashboard')).toBe(true); + expect(isNavItemActive('/dashboard/x', '/dashboard')).toBe(false); + expect(isNavItemActive('/vacay/2026', '/vacay')).toBe(true); + expect(isNavItemActive('/atlas', '/vacay')).toBe(false); + }); +}); diff --git a/client/src/components/Layout/useCreateAction.ts b/client/src/components/Layout/useCreateAction.ts new file mode 100644 index 000000000..6501e3204 --- /dev/null +++ b/client/src/components/Layout/useCreateAction.ts @@ -0,0 +1,75 @@ +import { useMatch, useNavigate } from 'react-router'; + +import { useTranslation } from '../../i18n'; +import { useJourneyStore } from '../../store/journeyStore'; + +interface CreateAction { + label: string; + run: () => void; + upload?: boolean; +} + +/** + * The centre "+" of the bottom navs means something different per context: inside a + * trip it adds a place, on the journey list it starts a journey, inside a journey it + * adds an entry, everywhere else it creates a new trip. Pages pick the intent up from + * the query params. + * + * `phone` adds what only the phone dock offers: inside a journey whose gallery is open + * it uploads a photo, on the atlas it opens the country search, on collections it adds + * a place to the active list. + */ +export function useCreateAction(phone: boolean): CreateAction { + const navigate = useNavigate(); + const { t } = useTranslation(); + const galleryOpen = useJourneyStore((state) => state.mobileGalleryOpen); + const inTrip = useMatch('/trips/:id'); + const inJourney = useMatch('/journey/:id'); + const onJourneyList = useMatch('/journey'); + const onAtlas = useMatch('/atlas'); + const onCollections = useMatch('/collections'); + const inCollection = useMatch('/collections/:id'); + + if (inTrip) { + // The "+" is context-aware per active tab: Bookings → reservation, + // Transports → transport, Costs → expense. Tabs without a create modal + // (lists / files / collab) fall through to adding a place. #1349 + const id = inTrip.params.id; + const tripTab = typeof sessionStorage !== 'undefined' ? sessionStorage.getItem(`trip-tab-${id}`) : null; + if (tripTab === 'finanzplan') + return { label: t('costs.addExpense'), run: () => navigate(`/trips/${id}?create=expense`) }; + if (tripTab === 'buchungen') + return { label: t('reservations.addManual'), run: () => navigate(`/trips/${id}?create=reservation`) }; + if (tripTab === 'transports') + return { label: t('transport.addManual'), run: () => navigate(`/trips/${id}?create=transport`) }; + return { label: t('places.addPlace'), run: () => navigate(`/trips/${id}?create=place`) }; + } + if (inJourney) { + // Context-aware per tab, like the trip's "+": the Gallery holds photos, so + // there the one big action is uploading one. + const journeyId = inJourney.params.id; + if (phone && galleryOpen) { + return { label: t('common.upload'), run: () => navigate(`/journey/${journeyId}?create=photo`), upload: true }; + } + return { label: t('journey.detail.addEntry'), run: () => navigate(`/journey/${journeyId}?create=entry`) }; + } + if (onJourneyList) { + return { label: t('journey.new'), run: () => navigate('/journey?create=1') }; + } + if (phone && onAtlas) { + return { label: t('atlas.searchCountry'), run: () => navigate('/atlas?search=1') }; + } + if (phone && (onCollections || inCollection)) { + // Picking a list moves the route to /collections/:id, so the exact match + // alone dropped the "+" through to creating a trip (#1930). The handoff keeps + // the id, otherwise adding would land on "All saved". + const path = inCollection ? `/collections/${inCollection.params.id}` : '/collections'; + return { label: t('collections.addPlace'), run: () => navigate(`${path}?create=place`) }; + } + return { label: t('dashboard.newTrip'), run: () => navigate('/dashboard?create=1') }; +} + +/** Whether a nav item is the current route: the dashboard only on its exact path, the rest by prefix. */ +export function isNavItemActive(pathname: string, to: string): boolean { + return to === '/dashboard' ? pathname === '/dashboard' : pathname.startsWith(to); +} diff --git a/client/src/mobile/components/MBottomNav.tsx b/client/src/mobile/components/MBottomNav.tsx index ad1bc3830..7a7ecb8de 100644 --- a/client/src/mobile/components/MBottomNav.tsx +++ b/client/src/mobile/components/MBottomNav.tsx @@ -1,75 +1,16 @@ import { useEffect, useState } from 'react' -import { useNavigate, useLocation, useMatch } from 'react-router' +import { useNavigate, useLocation } from 'react-router' import { useSettingsStore } from '../../store/settingsStore' import { useTranslation } from '../../i18n' -import { useJourneyStore } from '../../store/journeyStore' import { ChevronRight, MoreHorizontal, Plus, Search, Upload } from 'lucide-react' import type { LucideIcon } from 'lucide-react' import { normalizeAppearance } from '@trek/shared' import { useNavItems, splitMobileNav } from '../../components/Layout/navItems' +import { isNavItemActive, useCreateAction } from '../../components/Layout/useCreateAction' import MFab from './MFab' interface NavItem { to: string; label: string; icon: LucideIcon } -// The centre "+" means something different per context: inside a trip it adds a -// place, on the journey list it starts a journey (deliberate deviation from the -// demo, which reserves the FAB for entries — the list has no journey to add -// into yet), inside a journey it adds an entry, on the atlas it opens the -// country search, on collections it adds a place to the active list — -// everywhere else it creates a new trip. Pages pick the intent up from the -// query params. The result is unused on /vacay: that screen draws its own centre -// FAB, so the dock yields the slot instead (see screenFabSlot below, #1811). -function useCreateAction(): { label: string; run: () => void; upload?: boolean } { - const navigate = useNavigate() - const { t } = useTranslation() - const galleryOpen = useJourneyStore(state => state.mobileGalleryOpen) - const inTrip = useMatch('/trips/:id') - const inJourney = useMatch('/journey/:id') - const onJourneyList = useMatch('/journey') - const onAtlas = useMatch('/atlas') - const onCollections = useMatch('/collections') - const inCollection = useMatch('/collections/:id') - - if (inTrip) { - // The "+" is context-aware per active tab: Bookings → reservation, - // Transports → transport, Costs → expense. Tabs without a create modal - // (lists / files / collab) fall through to adding a place. #1349 - const id = inTrip.params.id - const tripTab = typeof sessionStorage !== 'undefined' ? sessionStorage.getItem(`trip-tab-${id}`) : null - if (tripTab === 'finanzplan') return { label: t('costs.addExpense'), run: () => navigate(`/trips/${id}?create=expense`) } - if (tripTab === 'buchungen') return { label: t('reservations.addManual'), run: () => navigate(`/trips/${id}?create=reservation`) } - if (tripTab === 'transports') return { label: t('transport.addManual'), run: () => navigate(`/trips/${id}?create=transport`) } - return { label: t('places.addPlace'), run: () => navigate(`/trips/${id}?create=place`) } - } - if (inJourney) { - // Context-aware per tab, like the trip's "+": the Gallery holds photos, so - // there the one big action is uploading one. Read from sessionStorage - // because the tab is the screen's own state and the dock is a sibling — - // exactly how the trip tabs hand theirs over. - const journeyId = inJourney.params.id - if (galleryOpen) { - return { label: t('common.upload'), run: () => navigate(`/journey/${journeyId}?create=photo`), upload: true } - } - return { label: t('journey.detail.addEntry'), run: () => navigate(`/journey/${journeyId}?create=entry`) } - } - if (onJourneyList) { - return { label: t('journey.new'), run: () => navigate('/journey?create=1') } - } - if (onAtlas) { - return { label: t('atlas.searchCountry'), run: () => navigate('/atlas?search=1') } - } - if (onCollections || inCollection) { - // Picking a list moves the route to /collections/:id, so the exact match - // alone dropped the "+" through to creating a trip — the one state the - // screen is normally used in (#1930). The handoff keeps the id, otherwise - // adding would land on "All saved" and the sheet would ask for the list the - // user is already looking at. - const path = inCollection ? `/collections/${inCollection.params.id}` : '/collections' - return { label: t('collections.addPlace'), run: () => navigate(`${path}?create=place`) } - } - return { label: t('dashboard.newTrip'), run: () => navigate('/dashboard?create=1') } -} - /** * Floating glass dock of the mobile shell. Same tab/gating/"+" logic as the * legacy BottomNav (addons, ?create= contract), redesigned as the demo's icon @@ -81,7 +22,9 @@ export default function MBottomNav() { const { t } = useTranslation() const navigate = useNavigate() const location = useLocation() - const create = useCreateAction() + // The "+" result is unused on /vacay: that screen draws its own centre FAB, so + // the dock yields the slot instead (see screenFabSlot below, #1811). + const create = useCreateAction(true) const [moreOpen, setMoreOpen] = useState(false) // Close the popover when the route changes underneath it (browser back etc.). @@ -97,8 +40,7 @@ export default function MBottomNav() { const dockItems: NavItem[] = split.bar const moreItems: NavItem[] = split.more - const isActive = (to: string) => - to === '/dashboard' ? location.pathname === '/dashboard' : location.pathname.startsWith(to) + const isActive = (to: string) => isNavItemActive(location.pathname, to) const moreActive = moreItems.some(item => isActive(item.to)) // The FAB gives way to a decorative logo slot on screens without an add