mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(maps): let an uploaded place image fill its marker
A place with an uploaded image showed a plain disc of its category colour on the map, while a picked suggestion looked fine. The marker sized its img with width and height attributes, and stylesheet rules beat those: Tailwind's preflight gives every img height: auto, and Leaflet's marker pane adds width: auto. Suggested and auto photos reach the marker as 48px square thumbs, so nothing showed. A phone photo kept its own proportions on the GL map, a sliver inside the circle, and its full pixel size on Leaflet, where only the rounded-off corner of a 4000px image lands inside the circle. Both maps now build the img through one helper that sizes it in its style, so object-fit: cover crops any photo to the circle. The thumb loading on both maps was keyed on place ids, so a change of picture under the same place went unnoticed until a reload: - Taking an upload off a place left the category dot instead of bringing its auto photo back. - Picking another suggested photo kept the thumb of the first one, which was cached under the provider id. A picked photo is now its own cache key.
This commit is contained in:
@@ -1180,6 +1180,31 @@ describe('MapView photo thumbnails', () => {
|
||||
expect(thumbCallbacks.size).toBe(0)
|
||||
expect(vi.mocked(photoService.fetchPhoto)).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('FE-COMP-MAPVIEW-080: an uploaded photo fills its marker whatever its proportions', () => {
|
||||
// Leaflet's marker pane sets `width: auto` on every img in it, so a photo sized
|
||||
// by attributes was drawn at its full pixel size and the circle only ever showed
|
||||
// the transparent corner of it, over the category colour.
|
||||
render(<MapView places={[buildMapPlace({ id: 27, lat: 48, lng: 2, image_url: '/uploads/places/wide.jpg' })]} />)
|
||||
const holder = document.createElement('div')
|
||||
holder.innerHTML = iconHtmlOf(screen.getAllByTestId('marker')[0])
|
||||
const img = holder.querySelector('img')!
|
||||
|
||||
expect(img.getAttribute('src')).toBe('/uploads/places/wide.jpg')
|
||||
expect(img.style.width).toBe('100%')
|
||||
expect(img.style.height).toBe('100%')
|
||||
})
|
||||
|
||||
it('FE-COMP-MAPVIEW-081: taking the upload off a place asks for its auto photo again, without a reload', () => {
|
||||
const place = buildMapPlace({ id: 28, lat: 48, lng: 2, google_place_id: 'gp-28', name: 'Tower', image_url: '/uploads/places/own.jpg' })
|
||||
const { rerender } = render(<MapView places={[place]} />)
|
||||
expect(vi.mocked(photoService.fetchPhoto)).not.toHaveBeenCalled()
|
||||
|
||||
rerender(<MapView places={[{ ...place, image_url: null }]} />)
|
||||
|
||||
expect(vi.mocked(photoService.fetchPhoto)).toHaveBeenCalledWith('gp-28', 'gp-28', 48, 2, 'Tower')
|
||||
expect(thumbCallbacks.has('gp-28')).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
// The marker HTML is a hand-built string handed to L.divIcon, i.e. innerHTML.
|
||||
|
||||
@@ -195,7 +195,7 @@ function createPlaceIcon(place, orderNumbers, isSelected) {
|
||||
box-shadow:${shadow};
|
||||
overflow:hidden;background:${bgColor};
|
||||
">
|
||||
<img src="${escapeHtml(place.image_url)}" width="${size}" height="${size}" style="display:block;border-radius:50%;object-fit:cover;" />
|
||||
${markerPhotoHtml(place.image_url)}
|
||||
</div>
|
||||
${badgeHtml}
|
||||
</div>`,
|
||||
@@ -543,7 +543,7 @@ function MapContextMenuHandler({ onContextMenu }: { onContextMenu: ((e: L.Leafle
|
||||
|
||||
// Module-level photo cache shared with PlaceAvatar
|
||||
import { getCached, isLoading, fetchPhoto, onThumbReady, getAllThumbs } from '../../services/photoService'
|
||||
import { isCustomPlaceImage, photoCacheKey } from './placePhoto'
|
||||
import { isCustomPlaceImage, markerPhotoHtml, photoCacheKey, photoSourcesKey } from './placePhoto'
|
||||
import { useAuthStore } from '../../store/authStore'
|
||||
import { useGeolocation } from '../../hooks/useGeolocation'
|
||||
import LocationButton from './LocationButton'
|
||||
@@ -838,7 +838,7 @@ export const MapView = memo(function MapView({
|
||||
const pendingThumbsRef = useRef<Record<string, string>>({})
|
||||
const thumbRafRef = useRef<number | null>(null)
|
||||
|
||||
const placeIds = useMemo(() => places.map(p => p.id).join(','), [places])
|
||||
const photoSources = useMemo(() => photoSourcesKey(places), [places])
|
||||
// Flattened [lat,lng] points of the selected day's route, so the bounds fit can
|
||||
// include the full polyline once it has been computed.
|
||||
const routeCoords = useMemo<[number, number][]>(() => (route || []).flat() as [number, number][], [route])
|
||||
@@ -895,7 +895,7 @@ export const MapView = memo(function MapView({
|
||||
thumbRafRef.current = null
|
||||
}
|
||||
}
|
||||
}, [placeIds, placesPhotosEnabled])
|
||||
}, [photoSources, placesPhotosEnabled])
|
||||
|
||||
const isTouchDevice = typeof window !== 'undefined' && navigator.maxTouchPoints > 0
|
||||
// Drag a marker onto a day (#891). Pointer-driven, so it is off wherever
|
||||
|
||||
@@ -1735,7 +1735,7 @@ describe('MapViewGL', () => {
|
||||
expect(srcs()).toContain('data:image/png;base64,BBB')
|
||||
})
|
||||
|
||||
it('FE-COMP-MAPVIEWGL-056: an in-flight photo is not requested twice and the proxy url is used as the id', async () => {
|
||||
it('FE-COMP-MAPVIEWGL-056: an in-flight photo is not requested twice and a picked proxy url is both id and cache key', async () => {
|
||||
vi.mocked(photoService.isLoading).mockImplementation((key: string) => key === 'osm-loading')
|
||||
const places = [
|
||||
buildMapPlace({ id: 94, lat: 48.4, lng: 2.4, osm_id: 'osm-loading' }),
|
||||
@@ -1746,7 +1746,56 @@ describe('MapViewGL', () => {
|
||||
await act(async () => {})
|
||||
|
||||
expect(photoService.fetchPhoto).toHaveBeenCalledTimes(1)
|
||||
expect(photoService.fetchPhoto).toHaveBeenCalledWith('osm-5', '/api/maps/place-photo/abc', 48.5, 2.5, 'Museum')
|
||||
expect(photoService.fetchPhoto).toHaveBeenCalledWith('/api/maps/place-photo/abc', '/api/maps/place-photo/abc', 48.5, 2.5, 'Museum')
|
||||
})
|
||||
|
||||
it('FE-COMP-MAPVIEWGL-101: an uploaded photo fills its marker whatever its proportions', async () => {
|
||||
// A phone photo is no 48px square like the thumbs. Sized by attributes it lost to
|
||||
// the stylesheet's `height: auto` and sat as a sliver in a disc of category colour.
|
||||
loadOnAttach()
|
||||
render(<MapViewGL places={[buildMapPlace({ id: 14, lat: 48.14, lng: 2.14, image_url: '/uploads/places/wide.jpg' })]} fitKey={1} />)
|
||||
await act(async () => {})
|
||||
|
||||
const img = glMarkers.created[0].element.querySelector('img')!
|
||||
expect(img.getAttribute('src')).toBe('/uploads/places/wide.jpg')
|
||||
expect(img.style.width).toBe('100%')
|
||||
expect(img.style.height).toBe('100%')
|
||||
})
|
||||
|
||||
it('FE-COMP-MAPVIEWGL-102: taking the upload off a place asks for its auto photo again, without a reload', async () => {
|
||||
const place = buildMapPlace({ id: 15, lat: 48.15, lng: 2.15, google_place_id: 'gp-15', name: 'Tower', image_url: '/uploads/places/own.jpg' })
|
||||
const { rerender } = render(<MapViewGL places={[place]} fitKey={1} />)
|
||||
await act(async () => {})
|
||||
expect(photoService.fetchPhoto).not.toHaveBeenCalled()
|
||||
|
||||
rerender(<MapViewGL places={[{ ...place, image_url: null }]} fitKey={1} />)
|
||||
await act(async () => {})
|
||||
|
||||
expect(photoService.fetchPhoto).toHaveBeenCalledWith('gp-15', 'gp-15', 48.15, 2.15, 'Tower')
|
||||
})
|
||||
|
||||
it('FE-COMP-MAPVIEWGL-103: picking another suggested photo replaces the thumb of the old one', async () => {
|
||||
loadOnAttach()
|
||||
const first = '/api/maps/place-photo/gp-16~p0/bytes'
|
||||
const second = '/api/maps/place-photo/gp-16~p1/bytes'
|
||||
// Everything seen before the new pick has a thumb: the place's auto photo under
|
||||
// its provider id and the first pick under its own url.
|
||||
vi.mocked(photoService.getCached).mockImplementation((key: string) => (
|
||||
key === 'gp-16' || key === first
|
||||
? ({ thumbDataUrl: 'data:image/png;base64,OLD' } as unknown as ReturnType<typeof photoService.getCached>)
|
||||
: undefined
|
||||
))
|
||||
const place = buildMapPlace({ id: 16, lat: 48.16, lng: 2.16, google_place_id: 'gp-16', name: 'Tower', image_url: first })
|
||||
const { rerender } = render(<MapViewGL places={[place]} fitKey={1} />)
|
||||
await flushFrames()
|
||||
|
||||
rerender(<MapViewGL places={[{ ...place, image_url: second }]} fitKey={1} />)
|
||||
await flushFrames()
|
||||
|
||||
expect(photoService.fetchPhoto).toHaveBeenCalledWith(second, second, 48.16, 2.16, 'Tower')
|
||||
// Until its thumb is ready the marker shows the new pick itself, not the old thumb.
|
||||
const latest = glMarkers.created[glMarkers.created.length - 1]
|
||||
expect(latest.element.querySelector('img')?.getAttribute('src')).toBe(second)
|
||||
})
|
||||
|
||||
it('FE-COMP-MAPVIEWGL-066: a place with neither provider id nor coordinates has no cache key and is skipped', async () => {
|
||||
|
||||
@@ -12,7 +12,7 @@ import { useTranslation } from '../../i18n/TranslationContext'
|
||||
import { MapLayerSwitcher, MAP_LAYER_SWITCHER_INSET, type BaseLayer } from './MapLayerSwitcher'
|
||||
import { useAuthStore } from '../../store/authStore'
|
||||
import { getCached, isLoading, fetchPhoto, onThumbReady, getAllThumbs } from '../../services/photoService'
|
||||
import { isCustomPlaceImage, photoCacheKey } from './placePhoto'
|
||||
import { isCustomPlaceImage, markerPhotoHtml, photoCacheKey, photoSourcesKey } from './placePhoto'
|
||||
import { CATEGORY_ICON_MAP } from '../shared/categoryIcons'
|
||||
import { isStandardFamily, supportsCustom3d, wantsTerrain, addCustom3dBuildings, addTerrainAndSky } from './mapboxSetup'
|
||||
import { attachLocationMarker, type LocationMarkerHandle } from './locationMarkerMapbox'
|
||||
@@ -20,7 +20,6 @@ import { ReservationMapboxOverlay } from './reservationsMapbox'
|
||||
import { useTransportRoutes } from '../../hooks/useTransportRoutes'
|
||||
import { visibleRouteReservations } from '../../utils/reservationRoutes'
|
||||
import { safeHexColor } from '../../utils/safeColor'
|
||||
import { escapeHtml } from '@trek/shared'
|
||||
import { MAPBOX_DEFAULT_STYLE, styleForActiveProvider, basemapLanguage, type GlMapProvider } from './glProviders'
|
||||
import LocationButton from './LocationButton'
|
||||
import { useGeolocation } from '../../hooks/useGeolocation'
|
||||
@@ -406,7 +405,7 @@ function createMarkerElement(place: Place & { category_color?: string; category_
|
||||
overflow:hidden;background:${bgColor};
|
||||
box-sizing:content-box;
|
||||
">
|
||||
<img src="${escapeHtml(photoUrl)}" width="${size}" height="${size}" style="display:block;border-radius:50%;object-fit:cover;" />
|
||||
${markerPhotoHtml(photoUrl)}
|
||||
</div>
|
||||
${badgeHtml}
|
||||
`
|
||||
@@ -1720,7 +1719,7 @@ export function MapViewGL({
|
||||
// simultaneous thumb arrivals into one re-render.
|
||||
const pendingThumbsRef = useRef<Record<string, string>>({})
|
||||
const thumbRafRef = useRef<number | null>(null)
|
||||
const placeIds = useMemo(() => places.map(p => p.id).join(','), [places])
|
||||
const photoSources = useMemo(() => photoSourcesKey(places), [places])
|
||||
useEffect(() => {
|
||||
if (!places || places.length === 0 || !placesPhotosEnabled) return
|
||||
const cleanups: (() => void)[] = []
|
||||
@@ -1771,7 +1770,7 @@ export function MapViewGL({
|
||||
thumbRafRef.current = null
|
||||
}
|
||||
}
|
||||
}, [placeIds, placesPhotosEnabled]) // eslint-disable-line react-hooks/exhaustive-deps
|
||||
}, [photoSources, placesPhotosEnabled]) // eslint-disable-line react-hooks/exhaustive-deps
|
||||
|
||||
// Reconcile markers with places + photos. The clustered GeoJSON source decides
|
||||
// which points are currently unclustered, and we render the existing rich HTML
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
import { describe, it, expect } from 'vitest'
|
||||
import { isCustomPlaceImage, markerPhotoHtml, photoCacheKey, photoSourcesKey } from './placePhoto'
|
||||
|
||||
function imgOf(html: string): HTMLImageElement {
|
||||
const holder = document.createElement('div')
|
||||
holder.innerHTML = html
|
||||
return holder.querySelector('img')!
|
||||
}
|
||||
|
||||
describe('isCustomPlaceImage', () => {
|
||||
it('FE-MAP-PLACEPHOTO-001: an upload or an inline image is shown as it is, a provider photo is not', () => {
|
||||
expect(isCustomPlaceImage('/uploads/places/a.jpg')).toBe(true)
|
||||
expect(isCustomPlaceImage('data:image/png;base64,AAA')).toBe(true)
|
||||
expect(isCustomPlaceImage('/api/maps/place-photo/gp-1~p0/bytes')).toBe(false)
|
||||
expect(isCustomPlaceImage(null)).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('markerPhotoHtml', () => {
|
||||
it('FE-MAP-PLACEPHOTO-002: sizes the picture by its style, where no stylesheet rule can undo it', () => {
|
||||
// Preflight's `height: auto` and Leaflet's `width: auto` both outrank a width or
|
||||
// height attribute, so a photo that is not a small square has to be pinned here.
|
||||
const img = imgOf(markerPhotoHtml('/uploads/places/wide.jpg'))
|
||||
expect(img.getAttribute('src')).toBe('/uploads/places/wide.jpg')
|
||||
expect(img.style.width).toBe('100%')
|
||||
expect(img.style.height).toBe('100%')
|
||||
expect(img.hasAttribute('width')).toBe(false)
|
||||
expect(img.hasAttribute('height')).toBe(false)
|
||||
})
|
||||
|
||||
it('FE-MAP-PLACEPHOTO-003: escapes the url, which a user can set', () => {
|
||||
const html = markerPhotoHtml('/uploads/x" onerror="alert(1)" y="')
|
||||
expect(html).not.toContain('onerror="alert(1)"')
|
||||
expect(imgOf(html).getAttribute('src')).toBe('/uploads/x" onerror="alert(1)" y="')
|
||||
})
|
||||
})
|
||||
|
||||
describe('photoCacheKey', () => {
|
||||
it('FE-MAP-PLACEPHOTO-004: a picked provider photo is its own key, ahead of the provider id', () => {
|
||||
expect(photoCacheKey({ image_url: '/api/maps/place-photo/gp-1~p2/bytes', google_place_id: 'gp-1', lat: 1, lng: 2 }))
|
||||
.toBe('/api/maps/place-photo/gp-1~p2/bytes')
|
||||
})
|
||||
|
||||
it('FE-MAP-PLACEPHOTO-005: otherwise the provider id, then the coordinates, then nothing', () => {
|
||||
expect(photoCacheKey({ image_url: '/uploads/places/a.jpg', google_place_id: 'gp-1', osm_id: 'node/1' })).toBe('gp-1')
|
||||
expect(photoCacheKey({ osm_id: 'node/1', lat: 1, lng: 2 })).toBe('node/1')
|
||||
expect(photoCacheKey({ lat: 0, lng: 0 })).toBe('0,0')
|
||||
expect(photoCacheKey({ image_url: 'https://example.com/a.jpg', lat: null, lng: null })).toBe('')
|
||||
})
|
||||
})
|
||||
|
||||
describe('photoSourcesKey', () => {
|
||||
const place = { id: 7, google_place_id: 'gp-7', lat: 48, lng: 2 }
|
||||
|
||||
it('FE-MAP-PLACEPHOTO-006: changes when an upload comes off or another photo is picked', () => {
|
||||
const withUpload = photoSourcesKey([{ ...place, image_url: '/uploads/places/a.jpg' }])
|
||||
const bare = photoSourcesKey([{ ...place, image_url: null }])
|
||||
const picked = photoSourcesKey([{ ...place, image_url: '/api/maps/place-photo/gp-7~p0/bytes' }])
|
||||
const pickedAgain = photoSourcesKey([{ ...place, image_url: '/api/maps/place-photo/gp-7~p1/bytes' }])
|
||||
expect(new Set([withUpload, bare, picked, pickedAgain]).size).toBe(4)
|
||||
})
|
||||
|
||||
it('FE-MAP-PLACEPHOTO-007: stays put when one upload replaces another, which needs no thumb', () => {
|
||||
expect(photoSourcesKey([{ ...place, image_url: '/uploads/places/a.jpg' }]))
|
||||
.toBe(photoSourcesKey([{ ...place, image_url: '/uploads/places/b.jpg' }]))
|
||||
})
|
||||
})
|
||||
@@ -1,3 +1,16 @@
|
||||
import { escapeHtml } from '@trek/shared'
|
||||
|
||||
const PHOTO_PROXY_PREFIX = '/api/maps/place-photo/'
|
||||
|
||||
/** The fields of a place that decide which picture its marker gets. */
|
||||
interface PhotoSource {
|
||||
image_url?: string | null
|
||||
google_place_id?: string | null
|
||||
osm_id?: string | null
|
||||
lat?: number | null
|
||||
lng?: number | null
|
||||
}
|
||||
|
||||
/**
|
||||
* A place's image_url that is directly displayable on a map marker and should
|
||||
* win over the auto-fetched provider thumbnail — a custom uploaded image (#1136)
|
||||
@@ -11,18 +24,41 @@ export function isCustomPlaceImage(url: string | null | undefined): boolean {
|
||||
|
||||
/**
|
||||
* Cache key under which a place's auto-fetched thumbnail is stored: the provider
|
||||
* id when there is one, otherwise the coordinates. A place with neither has no
|
||||
* identity to cache under, so it gets the empty string — callers skip it rather
|
||||
* photo the user picked when there is one, else the provider id, otherwise the
|
||||
* coordinates. The picked photo comes first because picking another picture of the
|
||||
* same place leaves its provider id alone, and the thumb cached under that id
|
||||
* would go on answering for the new one. A place with none of these has no
|
||||
* identity to cache under, so it gets the empty string: callers skip it rather
|
||||
* than filing every coordinate-less place under one shared entry.
|
||||
*/
|
||||
export function photoCacheKey(place: {
|
||||
google_place_id?: string | null
|
||||
osm_id?: string | null
|
||||
lat?: number | null
|
||||
lng?: number | null
|
||||
}): string {
|
||||
export function photoCacheKey(place: PhotoSource): string {
|
||||
if (place.image_url?.startsWith(PHOTO_PROXY_PREFIX)) return place.image_url
|
||||
if (place.google_place_id) return place.google_place_id
|
||||
if (place.osm_id) return place.osm_id
|
||||
if (place.lat == null || place.lng == null) return ''
|
||||
return `${place.lat},${place.lng}`
|
||||
}
|
||||
|
||||
/**
|
||||
* What the maps' thumb loading depends on, as one string to key the effect on.
|
||||
* Place ids alone missed every change of picture: taking an upload off a place or
|
||||
* picking another suggested photo changes no id, so the marker kept a category
|
||||
* dot or the old thumb until the page was reloaded.
|
||||
*/
|
||||
export function photoSourcesKey(places: Array<PhotoSource & { id: number }>): string {
|
||||
return places.map(p => `${p.id}:${isCustomPlaceImage(p.image_url) ? 'own' : photoCacheKey(p)}`).join('|')
|
||||
}
|
||||
|
||||
/**
|
||||
* The picture inside a round place marker, for both the Leaflet and the GL map.
|
||||
*
|
||||
* Sized by its style and not by width/height attributes, because stylesheet rules
|
||||
* beat attributes: Tailwind's preflight gives every img `height: auto`, and
|
||||
* Leaflet's marker pane adds `width: auto`. The 48px square thumbs got away with
|
||||
* that. An uploaded photo kept its own proportions on the GL map and its full pixel
|
||||
* size on Leaflet, where only the rounded-off corner of a 4000px image lands inside
|
||||
* the circle, so the marker showed nothing but its category colour.
|
||||
*/
|
||||
export function markerPhotoHtml(url: string): string {
|
||||
return `<img src="${escapeHtml(url)}" alt="" style="display:block;width:100%;height:100%;border-radius:50%;object-fit:cover;" />`
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user