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() + }) + }) })