From db6bb911c20a818d10b5080abffc579b226ee8d8 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 13 Nov 2026 09:28:30 +0100 Subject: [PATCH] feat(roadtrip): show the place photo on the stop sheet and the stop dialog (#2629) On the road a stop's picture is often what decides whether it is worth the detour. The phone's stop sheet shows the place's picture full width under its name and the desktop stop dialog shows it in place of the kind tile; both open the place's photos on tap. The picture is the place's own image, else the provider photo for its Google or OSM id through the shared photo cache. --- .../Roadtrip/RoadtripStopPopup.test.tsx | 27 +++++++- .../components/Roadtrip/RoadtripStopPopup.tsx | 24 +++++-- client/src/components/Roadtrip/StopPhoto.tsx | 49 ++++++++++++++ .../components/shared/usePlacePhoto.test.ts | 65 +++++++++++++++++++ client/src/components/shared/usePlacePhoto.ts | 52 +++++++++++++++ .../screens/trip/roadtrip/MRtStopSheet.tsx | 6 ++ .../unit/mobile/trip/MRtStopSheet.test.tsx | 18 ++++- 7 files changed, 231 insertions(+), 10 deletions(-) create mode 100644 client/src/components/Roadtrip/StopPhoto.tsx create mode 100644 client/src/components/shared/usePlacePhoto.test.ts create mode 100644 client/src/components/shared/usePlacePhoto.ts diff --git a/client/src/components/Roadtrip/RoadtripStopPopup.test.tsx b/client/src/components/Roadtrip/RoadtripStopPopup.test.tsx index f9764950e..db358c033 100644 --- a/client/src/components/Roadtrip/RoadtripStopPopup.test.tsx +++ b/client/src/components/Roadtrip/RoadtripStopPopup.test.tsx @@ -8,8 +8,10 @@ import type { CorridorPoi } from './useCorridorPois' // The dialog carries the charging panel, which reads the open availability data for a // station that is not on the trip yet. Both of its collaborators are stubbed: the trip -// is whichever one is open, and the data comes off the network. -vi.mock('../../store/tripStore', () => ({ useTripStore: (select: (s: { trip: { id: number } }) => unknown) => select({ trip: { id: 1 } }) })) +// is whichever one is open, and the data comes off the network. The trip's places are +// what a stop being edited shows its picture from (#2629). +const tripState = vi.hoisted(() => ({ trip: { id: 1 }, places: [] as Array> })) +vi.mock('../../store/tripStore', () => ({ useTripStore: (select: (s: typeof tripState) => unknown) => select(tripState) })) vi.mock('../../repo/chargingRepo', () => ({ chargingRepo: { read: vi.fn(), readAt: vi.fn() } })) const wrap = (ui: React.ReactElement) => render({ui}) @@ -21,7 +23,10 @@ const charging = { tariffs: [], pricesUnavailable: false, } -beforeEach(() => { vi.clearAllMocks() }) +beforeEach(() => { + vi.clearAllMocks() + tripState.places = [] +}) function poi(over: Partial = {}): CorridorPoi { return { @@ -394,6 +399,22 @@ describe('editing a roadtrip stop', () => { expect(screen.getByRole('button', { name: 'Overnight' })).toHaveAttribute('aria-pressed', 'true') expect(screen.getByRole('textbox', { name: 'Check-in' })).toHaveValue('15:45') }) + + it('FE-ROADTRIP-STOPPOPUP-035: a stop on the trip shows its picture in place of the tile, and it opens the photos (#2629)', () => { + tripState.places = [{ id: 7, name: 'Aral Autohof', lat: 53.1, lng: 10.2, image_url: '/uploads/places/aral.jpg' }] + wrap() + + const trigger = screen.getByRole('button', { name: 'Show photos' }) + expect(trigger.querySelector('img')).toHaveAttribute('src', '/uploads/places/aral.jpg') + }) + + it('FE-ROADTRIP-STOPPOPUP-036: a hit from the search is no place yet and keeps the tile of its kind', () => { + tripState.places = [{ id: 7, name: 'Aral Autohof', lat: 53.1, lng: 10.2, image_url: '/uploads/places/aral.jpg' }] + const { container } = wrap() + + expect(screen.queryByRole('button', { name: 'Show photos' })).toBeNull() + expect(container.ownerDocument.querySelector('img[src="/uploads/places/aral.jpg"]')).toBeNull() + }) }) diff --git a/client/src/components/Roadtrip/RoadtripStopPopup.tsx b/client/src/components/Roadtrip/RoadtripStopPopup.tsx index 11fa464e9..cba51d02a 100644 --- a/client/src/components/Roadtrip/RoadtripStopPopup.tsx +++ b/client/src/components/Roadtrip/RoadtripStopPopup.tsx @@ -5,7 +5,9 @@ import { PANEL, Segmented } from '../shared/dialogParts' import CustomTimePicker from '../shared/CustomTimePicker' import StayPortals from './StayPortals' import ChargingInfo from './ChargingInfo' +import StopPhoto from './StopPhoto' import { useTranslation } from '../../i18n/TranslationContext' +import { useTripStore } from '../../store/tripStore' import { safeExternalHref } from '../../utils/safeUrl' import { StopKindChips, StopStayChips } from './StopChips' import type { CorridorPoi } from './useCorridorPois' @@ -94,6 +96,10 @@ export default function RoadtripStopPopup({ const [night, setNight] = useState(() => nightByDefault(draft)) const [checkIn, setCheckIn] = useState(() => checkInByDefault(draft)) const labelId = useId() + // A stop already on the trip shows its picture in place of the kind's tile (#2629); a + // hit from the search is not a place yet and keeps the tile. + const editedPlaceId = draft?.editing?.placeId + const place = useTripStore(s => (editedPlaceId == null ? null : s.places.find(p => p.id === editedPlaceId) ?? null)) // The popup stays mounted while the draft is swapped for another one, so every // answer is seeded again from the new draft, the way the phone sheet does it. @@ -137,12 +143,18 @@ export default function RoadtripStopPopup({ header={( - {/* The road-trip palette, the same one the map pin and the result row use. */} - - {kind ? : } - - + + {/* The road-trip palette, the same one the map pin and the result row use. */} + + {kind ? : } + + + )} + /> )} tint={NEUTRAL_TINT} labelId={labelId} diff --git a/client/src/components/Roadtrip/StopPhoto.tsx b/client/src/components/Roadtrip/StopPhoto.tsx new file mode 100644 index 000000000..83791c8fa --- /dev/null +++ b/client/src/components/Roadtrip/StopPhoto.tsx @@ -0,0 +1,49 @@ +import { useState, type ReactNode } from 'react'; + +import { useTranslation } from '../../i18n'; +import type { Place } from '../../types'; +import PlacePhotoGallery from '../Planner/PlacePhotoGallery'; +import { usePlacePhoto } from '../shared/usePlacePhoto'; + +interface StopPhotoProps { + /** The trip's place behind the stop; a stop not saved yet has none. */ + place: Place | null; + /** The frame: its size, corners and spacing. The picture fills it. */ + className: string; + /** What stands in while there is no picture. */ + fallback?: ReactNode; +} + +/** + * A stop's picture on the road (#2629): what helps decide, from the passenger + * seat, whether a place is worth the detour. A tap opens the place's photos, the + * same gallery the place sheet and the inspector open. A picture that fails to + * load gives way to the fallback rather than leaving a broken image behind. + */ +export default function StopPhoto({ place, className, fallback = null }: StopPhotoProps) { + const { t, language } = useTranslation(); + const photo = usePlacePhoto(place); + const [broken, setBroken] = useState(null); + + if (!place || !photo || photo === broken) return <>{fallback}; + // The corners are the frame's, inherited all the way down, so the focus ring of the + // gallery trigger follows the picture whatever shape the caller gave it. + return ( +
+ + setBroken(photo)} + className="block h-full w-full rounded-[inherit] object-cover" + /> + +
+ ); +} diff --git a/client/src/components/shared/usePlacePhoto.test.ts b/client/src/components/shared/usePlacePhoto.test.ts new file mode 100644 index 000000000..be9dc8b3b --- /dev/null +++ b/client/src/components/shared/usePlacePhoto.test.ts @@ -0,0 +1,65 @@ +// FE-SHARED-PLACEPHOTO-001 to FE-SHARED-PLACEPHOTO-005 +import { renderHook } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { resetAllStores, seedStore } from '../../../tests/helpers/store'; +import { fetchPhoto, getCached } from '../../services/photoService'; +import { useAuthStore } from '../../store/authStore'; +import { usePlacePhoto } from './usePlacePhoto'; + +vi.mock('../../services/photoService', () => ({ fetchPhoto: vi.fn(), getCached: vi.fn() })); + +const PROXY = '/api/maps/place-photo/ChIJ123/bytes'; + +beforeEach(() => { + resetAllStores(); + vi.clearAllMocks(); + vi.mocked(getCached).mockReturnValue(undefined); + vi.mocked(fetchPhoto).mockImplementation((_key, _id, _lat, _lng, _name, callback) => { + callback?.({ photoUrl: PROXY, thumbDataUrl: null }); + }); +}); + +describe('usePlacePhoto (#2629)', () => { + it("FE-SHARED-PLACEPHOTO-001: the place's own picture wins and asks the provider for nothing", () => { + const { result } = renderHook(() => + usePlacePhoto({ name: 'Dom', image_url: '/uploads/places/dom.jpg', google_place_id: 'ChIJ123' }) + ); + expect(result.current).toBe('/uploads/places/dom.jpg'); + expect(fetchPhoto).not.toHaveBeenCalled(); + }); + + it('FE-SHARED-PLACEPHOTO-002: without one, the provider picture is asked by its id through the shared cache', () => { + const { result } = renderHook(() => + usePlacePhoto({ name: 'Dom', google_place_id: 'ChIJ123', lat: 50.9, lng: 6.9 }) + ); + expect(fetchPhoto).toHaveBeenCalledWith('ChIJ123', 'ChIJ123', 50.9, 6.9, 'Dom', expect.any(Function)); + expect(result.current).toBe(PROXY); + }); + + it('FE-SHARED-PLACEPHOTO-003: a place known only by its position gets no picture of its surroundings', () => { + const { result } = renderHook(() => usePlacePhoto({ name: 'Somewhere', lat: 50.9, lng: 6.9 })); + expect(fetchPhoto).not.toHaveBeenCalled(); + expect(result.current).toBeNull(); + }); + + it('FE-SHARED-PLACEPHOTO-004: with photos switched off nothing is asked and nothing is shown', () => { + seedStore(useAuthStore, { placesPhotosEnabled: false }); + const { result } = renderHook(() => usePlacePhoto({ name: 'Dom', osm_id: 'way:42' })); + expect(fetchPhoto).not.toHaveBeenCalled(); + expect(result.current).toBeNull(); + }); + + it("FE-SHARED-PLACEPHOTO-005: the next place never shows the previous one's picture while its own loads", () => { + vi.mocked(fetchPhoto).mockImplementation((key, _id, _lat, _lng, _name, callback) => { + if (key === 'ChIJ123') callback?.({ photoUrl: PROXY, thumbDataUrl: null }); + }); + const { result, rerender } = renderHook(({ id }) => usePlacePhoto({ name: 'Stop', google_place_id: id }), { + initialProps: { id: 'ChIJ123' }, + }); + expect(result.current).toBe(PROXY); + + rerender({ id: 'ChIJ456' }); + expect(result.current).toBeNull(); + }); +}); diff --git a/client/src/components/shared/usePlacePhoto.ts b/client/src/components/shared/usePlacePhoto.ts new file mode 100644 index 000000000..bbb406e44 --- /dev/null +++ b/client/src/components/shared/usePlacePhoto.ts @@ -0,0 +1,52 @@ +import { useEffect, useState } from 'react'; + +import { fetchPhoto, getCached } from '../../services/photoService'; +import { useAuthStore } from '../../store/authStore'; +import { photoCacheKey } from '../Map/placePhoto'; + +/** What a place's picture is found by. */ +interface PhotoPlace { + name: string; + image_url?: string | null; + google_place_id?: string | null; + osm_id?: string | null; + lat?: number | null; + lng?: number | null; +} + +/** + * A place's picture at full size, for a view that shows it large (#2629). + * + * The place's own picture wins, as everywhere else. Without one, the picture its + * provider has for its Google or OSM id is asked through the shared photo cache, + * so a place the list or the map already shows costs no second request. A place + * known only by its position gets none: the proxy would answer with whatever is + * near those coordinates, the same street for every stop along it. Null while it + * loads, when there is none and while photos are switched off. + */ +export function usePlacePhoto(place: PhotoPlace | null): string | null { + const enabled = useAuthStore((s) => s.placesPhotosEnabled); + const own = place?.image_url || null; + const providerId = place?.google_place_id || place?.osm_id || null; + const key = place && !own && providerId ? photoCacheKey(place) : ''; + const lat = place?.lat ?? undefined; + const lng = place?.lng ?? undefined; + const name = place?.name; + // Names the place it belongs to, so the previous place's picture never shows for the next. + const [loaded, setLoaded] = useState<{ key: string; url: string | null } | null>(null); + + useEffect(() => { + if (!enabled || !key || !providerId) return; + let cancelled = false; + fetchPhoto(key, providerId, lat, lng, name, (entry) => { + if (!cancelled) setLoaded({ key, url: entry.photoUrl }); + }); + return () => { + cancelled = true; + }; + }, [enabled, key, providerId, lat, lng, name]); + + if (own) return own; + if (!enabled || !key) return null; + return loaded?.key === key ? loaded.url : (getCached(key)?.photoUrl ?? null); +} diff --git a/client/src/mobile/screens/trip/roadtrip/MRtStopSheet.tsx b/client/src/mobile/screens/trip/roadtrip/MRtStopSheet.tsx index 97fcba646..6b43c093e 100644 --- a/client/src/mobile/screens/trip/roadtrip/MRtStopSheet.tsx +++ b/client/src/mobile/screens/trip/roadtrip/MRtStopSheet.tsx @@ -11,6 +11,7 @@ import { useTranslation } from '../../../../i18n' import { useSettingsStore } from '../../../../store/settingsStore' import { useTripStore } from '../../../../store/tripStore' import ChargingInfo from '../../../../components/Roadtrip/ChargingInfo' +import StopPhoto from '../../../../components/Roadtrip/StopPhoto' import { formatDurationShort } from '../../../../components/Roadtrip/roadtripModel' import { roadtripRows, stageOf, type StopRow } from '../../../../components/Roadtrip/roadtripRowModel' import { readStay, shownStay } from '../../../../components/Roadtrip/stayReading' @@ -336,6 +337,11 @@ export default function MRtStopSheet({ planner, shell }: MTripSheetsProps) {
+ {/* ── The place's picture, which is often what decides whether a stop is worth it + when the day runs short (#2629). Full width under the name, where the eye lands + first; a tap opens the rest of its photos. Absent when the place has none. ── */} + + {/* ── The two clock readings, and where they come from ── */} {(arrive || leave) && (
diff --git a/client/tests/unit/mobile/trip/MRtStopSheet.test.tsx b/client/tests/unit/mobile/trip/MRtStopSheet.test.tsx index d23cdd740..a40a30e3d 100644 --- a/client/tests/unit/mobile/trip/MRtStopSheet.test.tsx +++ b/client/tests/unit/mobile/trip/MRtStopSheet.test.tsx @@ -8,7 +8,7 @@ import { useTripStore } from '../../../../src/store/tripStore' import { resetAllStores, seedStore } from '../../../helpers/store' import { fireEvent, render, screen, waitFor } from '../../../helpers/render' -// FE-MOB-RTSTOP-001 to FE-MOB-RTSTOP-038 +// FE-MOB-RTSTOP-001 to FE-MOB-RTSTOP-041 // // The sheet renders inside the real TranslationProvider, so the copy is asserted // in English. `planner.t` from the fixture is never consulted here. @@ -557,4 +557,20 @@ describe('MRtStopSheet', () => { expect(screen.getByText('The travel day ends before 14:00, so the drive goes on the next morning.')).toBeInTheDocument() }) }) + + describe('the place picture (#2629)', () => { + it('FE-MOB-RTSTOP-040: a place with a picture shows it under the name, and a tap opens its photos', () => { + const withPhoto = PLACES.map(p => (p.id === 203 ? { ...p, image_url: '/uploads/places/bremen.jpg' } : p)) + renderSheet({}, {}, withPhoto as Place[]) + const trigger = screen.getByRole('button', { name: 'Show photos' }) + expect(trigger.querySelector('img')).toHaveAttribute('src', '/uploads/places/bremen.jpg') + }) + + it('FE-MOB-RTSTOP-041: a place without one, or known only by its position, shows no picture and asks for none', () => { + renderSheet() + expect(screen.getByRole('dialog')).toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Show photos' })).not.toBeInTheDocument() + expect(screen.getByRole('dialog').querySelector('img')).toBeNull() + }) + }) })