mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-24 16:03:02 +00:00
Get the pictures right and stop reusing stale answers
Three things you could see on dev1. Pictures were "roughly right" rather than right, because a coordinate search only knows what was photographed within 300 m. Wikidata's P18 is one picture somebody chose to represent that exact object - "Zurletzteninstanz.jpg" for the restaurant, not a statue down the road - so it now leads, then the Commons category, and geosearch is the last resort it should always have been. Small venues carry a wikidata tag far more often than a Commons category, so this is also what reaches the places the other sources miss. Descriptions stayed on Wikipedia after the switch to Wikivoyage because the week-long result cache predates it. It carries a format version now; a release that changes the shape or the sources invalidates its own entries instead of serving last week's for another six days. Ratings and opening hours were being fetched from Google and then dropped on the floor - the fact list only looked at OpenStreetMap tags. Both are shown now, whichever provider supplied them. Also: the column follows a place opened from the map, not only one picked from search. A POI tap arrives as prefillCoords and never touched the selection, so the column kept showing the previously added place. Pictures are a tight three-column grid and only the picture in play is credited in full, on one line. Crediting all of them at two lines each drowned the column, and the licence attaches to the one that gets used - whose credit also stays visible in the inspector after saving. The request carries the place record the dialog already fetched, so the server skips a details lookup it was making in parallel with the client's own. That lookup was measured at 12.8 s for a large OSM relation.
This commit is contained in:
@@ -13,6 +13,7 @@ import {
|
||||
ScrollText,
|
||||
ShoppingBag,
|
||||
Sprout,
|
||||
Star,
|
||||
Sun,
|
||||
Wifi,
|
||||
} from 'lucide-react'
|
||||
@@ -26,6 +27,8 @@ export interface PlaceDetailsSelection {
|
||||
lat: number
|
||||
lng: number
|
||||
name: string
|
||||
/** The picked search result, so the server can skip its own details lookup. */
|
||||
details?: Record<string, unknown>
|
||||
}
|
||||
|
||||
interface PlaceDetailsColumnProps {
|
||||
@@ -128,7 +131,14 @@ export default function PlaceDetailsColumn({
|
||||
|
||||
mapsApi
|
||||
.placeEnrichment(
|
||||
{ placeId: selection.placeId, lat: selection.lat, lng: selection.lng, name: selection.name, lang: language },
|
||||
{
|
||||
placeId: selection.placeId,
|
||||
lat: selection.lat,
|
||||
lng: selection.lng,
|
||||
name: selection.name,
|
||||
lang: language,
|
||||
details: selection.details,
|
||||
},
|
||||
controller.signal,
|
||||
)
|
||||
.then((result) => {
|
||||
@@ -204,11 +214,17 @@ export default function PlaceDetailsColumn({
|
||||
}
|
||||
|
||||
/**
|
||||
* The picture strip: the first candidate wide, the rest as a row underneath.
|
||||
* The picture strip.
|
||||
*
|
||||
* A plain two-column grid left a hole whenever an odd number of pictures came
|
||||
* back, which is most of the time. A lead image also gives the column something
|
||||
* to look at instead of a page of equal squares.
|
||||
* A tight three-column grid rather than a lead image: this sits beside a form
|
||||
* in a dialog, and pictures that take half the column push the facts and the
|
||||
* description out of sight.
|
||||
*
|
||||
* Only the picture in play is credited in full. Crediting all of them at once
|
||||
* cost two lines each and drowned the column, and the licence obligation
|
||||
* attaches to the picture that gets used — which is the selected one, whose
|
||||
* credit also stays visible in the inspector after saving. Every tile still
|
||||
* carries the full credit as its tooltip and links to its source page.
|
||||
*/
|
||||
function PhotoStrip({
|
||||
photos,
|
||||
@@ -221,88 +237,95 @@ function PhotoStrip({
|
||||
onPickImage: (url: string | null) => void
|
||||
t: TranslationFn
|
||||
}): React.ReactElement | null {
|
||||
const [hovered, setHovered] = useState<string | null>(null)
|
||||
if (photos.length === 0) return null
|
||||
|
||||
const [lead, ...rest] = photos
|
||||
const shown = photos.find((p) => p.url === (hovered ?? selectedImageUrl)) ?? photos[0]
|
||||
|
||||
return (
|
||||
<div className="space-y-2">
|
||||
<Overline>{t('places.details.pickImage')}</Overline>
|
||||
<PhotoTile photo={lead} wide selected={selectedImageUrl === lead.url} onPick={onPickImage} t={t} />
|
||||
{rest.length > 0 && (
|
||||
<div className="grid grid-cols-3 gap-2">
|
||||
{rest.map((photo) => (
|
||||
<PhotoTile
|
||||
key={photo.key}
|
||||
photo={photo}
|
||||
selected={selectedImageUrl === photo.url}
|
||||
onPick={onPickImage}
|
||||
t={t}
|
||||
/>
|
||||
))}
|
||||
</div>
|
||||
)}
|
||||
<div className="grid grid-cols-3 gap-1.5">
|
||||
{photos.map((photo) => (
|
||||
<PhotoTile
|
||||
key={photo.key}
|
||||
photo={photo}
|
||||
selected={selectedImageUrl === photo.url}
|
||||
onPick={onPickImage}
|
||||
onHover={setHovered}
|
||||
t={t}
|
||||
/>
|
||||
))}
|
||||
</div>
|
||||
<PhotoCredit photo={shown} />
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
function PhotoTile({
|
||||
photo,
|
||||
wide = false,
|
||||
selected,
|
||||
onPick,
|
||||
onHover,
|
||||
t,
|
||||
}: {
|
||||
photo: PlacePhotoCandidate
|
||||
wide?: boolean
|
||||
selected: boolean
|
||||
onPick: (url: string | null) => void
|
||||
onHover: (url: string | null) => void
|
||||
t: TranslationFn
|
||||
}): React.ReactElement {
|
||||
const credit = photo.attribution || sourceLabelFor(photo.source)
|
||||
|
||||
return (
|
||||
<div className="space-y-1">
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => onPick(selected ? null : photo.url)}
|
||||
aria-pressed={selected}
|
||||
aria-label={photo.attribution ? `${t('places.details.pickImage')} — ${photo.attribution}` : t('places.details.pickImage')}
|
||||
className={`group relative block w-full overflow-hidden rounded-lg transition-shadow ${
|
||||
wide ? 'aspect-[16/10]' : 'aspect-square'
|
||||
} ${selected ? 'ring-2 ring-accent ring-offset-2 ring-offset-surface-secondary' : 'ring-1 ring-edge hover:ring-content-muted'}`}
|
||||
>
|
||||
<img
|
||||
src={photo.url}
|
||||
alt=""
|
||||
loading="lazy"
|
||||
className="w-full h-full object-cover transition-transform duration-500 group-hover:scale-[1.04]"
|
||||
/>
|
||||
{selected && (
|
||||
<span className="absolute top-1.5 right-1.5 rounded-full bg-accent p-1 shadow-card">
|
||||
<Check className="w-3 h-3 text-accent-on" />
|
||||
</span>
|
||||
)}
|
||||
</button>
|
||||
<PhotoCredit photo={photo} />
|
||||
</div>
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => onPick(selected ? null : photo.url)}
|
||||
onMouseEnter={() => onHover(photo.url)}
|
||||
onMouseLeave={() => onHover(null)}
|
||||
onFocus={() => onHover(photo.url)}
|
||||
onBlur={() => onHover(null)}
|
||||
aria-pressed={selected}
|
||||
aria-label={`${t('places.details.pickImage')} — ${credit}`}
|
||||
title={`${credit}${photo.license ? ` · ${photo.license}` : ''}`}
|
||||
className={`group relative block w-full aspect-square overflow-hidden rounded-lg transition-shadow ${
|
||||
selected ? 'ring-2 ring-accent ring-offset-2 ring-offset-surface-secondary' : 'ring-1 ring-edge hover:ring-content-muted'
|
||||
}`}
|
||||
>
|
||||
<img
|
||||
src={photo.url}
|
||||
alt=""
|
||||
loading="lazy"
|
||||
className="w-full h-full object-cover transition-transform duration-500 group-hover:scale-[1.06]"
|
||||
/>
|
||||
{selected && (
|
||||
<span className="absolute top-1 right-1 rounded-full bg-accent p-0.5 shadow-card">
|
||||
<Check className="w-2.5 h-2.5 text-accent-on" />
|
||||
</span>
|
||||
)}
|
||||
</button>
|
||||
)
|
||||
}
|
||||
|
||||
function sourceLabelFor(source: PlacePhotoCandidate['source']): string {
|
||||
if (source === 'google') return 'Google'
|
||||
if (source === 'wikipedia') return 'Wikipedia'
|
||||
return 'Wikimedia Commons'
|
||||
}
|
||||
|
||||
/**
|
||||
* Author and licence under every thumbnail.
|
||||
* Author and licence for the picture currently in play.
|
||||
*
|
||||
* Not decoration: Commons images are largely CC BY / CC BY-SA, and reusing one
|
||||
* without naming its author does not satisfy those terms. Wrapped over two
|
||||
* lines rather than truncated to one, because a cut-off name credits nobody.
|
||||
* When a source hands us no author (Google), we say where it came from rather
|
||||
* than inventing a credit.
|
||||
* without naming its author does not satisfy those terms. When a source hands
|
||||
* us no author (Google), we say where it came from rather than inventing one.
|
||||
*/
|
||||
function PhotoCredit({ photo }: { photo: PlacePhotoCandidate }): React.ReactElement {
|
||||
const sourceLabel = photo.source === 'google' ? 'Google' : photo.source === 'wikipedia' ? 'Wikipedia' : 'Wikimedia Commons'
|
||||
const credit = photo.attribution || sourceLabel
|
||||
const credit = photo.attribution || sourceLabelFor(photo.source)
|
||||
const full = `${credit}${photo.license ? ` · ${photo.license}` : ''}`
|
||||
|
||||
return (
|
||||
<p className="text-caption leading-tight text-content-faint line-clamp-2" title={full}>
|
||||
<p className="text-caption leading-tight text-content-faint truncate" title={full}>
|
||||
{photo.sourceUrl ? (
|
||||
<a href={photo.sourceUrl} target="_blank" rel="noopener noreferrer" className="hover:underline">
|
||||
{credit}
|
||||
@@ -327,6 +350,7 @@ function PhotoCredit({ photo }: { photo: PlacePhotoCandidate }): React.ReactElem
|
||||
}
|
||||
|
||||
const FACT_ICONS: Record<PlaceFact['kind'], typeof ChefHat> = {
|
||||
rating: Star,
|
||||
cuisine: ChefHat,
|
||||
openingHours: Clock,
|
||||
menu: ScrollText,
|
||||
|
||||
@@ -1014,6 +1014,21 @@ describe('PlaceFormModal', () => {
|
||||
expect(screen.getByText('screenshot.png')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('FE-PLANNER-PLACEFORM-057: a prefilled place drives the detail column, not the previous one', () => {
|
||||
// A POI tapped on the map opens the dialog through prefillCoords, never
|
||||
// through the search handler — the column used to keep the last place.
|
||||
const { rerender } = render(
|
||||
<PlaceFormModal {...defaultProps} prefillCoords={{ lat: 53.55, lng: 9.99, name: 'Helgas Kitchen', osm_id: 'node:1' }} />,
|
||||
);
|
||||
expect(screen.getByDisplayValue('Helgas Kitchen')).toBeInTheDocument();
|
||||
|
||||
rerender(
|
||||
<PlaceFormModal {...defaultProps} prefillCoords={{ lat: 50.9, lng: 6.96, name: 'Museum Ludwig', osm_id: 'node:2' }} />,
|
||||
);
|
||||
expect(screen.getByDisplayValue('Museum Ludwig')).toBeInTheDocument();
|
||||
expect(screen.queryByDisplayValue('Helgas Kitchen')).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('FE-PLANNER-PLACEFORM-055: pasting plain text alone attaches nothing', () => {
|
||||
render(<PlaceFormModal {...defaultProps} />);
|
||||
const form = document.querySelector('form') as HTMLFormElement;
|
||||
|
||||
@@ -138,6 +138,28 @@ function usePlaceFormModal(props: PlaceFormModalProps) {
|
||||
}
|
||||
setPendingFiles([])
|
||||
setDuplicateWarning(null)
|
||||
// The column follows whatever the dialog was opened with, not only a search
|
||||
// pick: a POI tapped on the map and a right-click place arrive as
|
||||
// prefillCoords, and editing an existing place arrives as `place`. Without
|
||||
// this the column kept showing the previous place's pictures and facts,
|
||||
// because only handleSelectMapsResult ever set it.
|
||||
if (place && place.lat != null && place.lng != null) {
|
||||
setDetailsSelection({
|
||||
placeId: place.google_place_id || place.osm_id || undefined,
|
||||
lat: Number(place.lat),
|
||||
lng: Number(place.lng),
|
||||
name: place.name || '',
|
||||
})
|
||||
} else if (prefillCoords) {
|
||||
setDetailsSelection({
|
||||
placeId: prefillCoords.osm_id || undefined,
|
||||
lat: prefillCoords.lat,
|
||||
lng: prefillCoords.lng,
|
||||
name: prefillCoords.name || '',
|
||||
})
|
||||
} else {
|
||||
setDetailsSelection(null)
|
||||
}
|
||||
// dayAssignments is a fresh array each render; read it at open-time only and
|
||||
// re-run on identity changes (place/assignmentId/open), not on every render.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
@@ -280,6 +302,9 @@ function usePlaceFormModal(props: PlaceFormModalProps) {
|
||||
lat,
|
||||
lng,
|
||||
name: result.name || '',
|
||||
// Hand the record along: the server needs the same OSM tags, and
|
||||
// looking them up again costs an Overpass round trip it can skip.
|
||||
details: result,
|
||||
})
|
||||
setForm(prev => ({ ...prev, image_url: undefined }))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user