Panorama labels - #1135
Conversation
Show the same dot markers and fruit-name labels in the Street View panorama as on the map, by extracting the map's marker/label rendering into a shared module (locationMarkerHelpers.js) and reusing it. The OverlayView labels attach to a StreetViewPanorama as well as a Map, so both views render identically. - All nearby locations get the map's dots (blue by default, orange when saved) plus their fruit-name label; labels use the satellite/hybrid style (white text, dark outline) since the panorama is imagery. - The selected/viewed location additionally gets the orange "here" pin above its dot, using the same boxicons Map glyph as the map's MapPin. - The map's own markers are hidden while Street View is open so they don't double up with the panorama's. - Guard against async re-entrancy so a superseded panorama can't leave orphaned overlays on the shared panorama. Closes falling-fruit#1081 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b418969 to
5ec1052
Compare
|
Looked at the code, looks mostly fine to me. ptal :) |
wbazant
left a comment
wbazant
left a comment
There was a problem hiding this comment.
Thanks! I had some comments to help the change fit better with the rest of the code base but it's otherwise all right and it looks like it will do the right thing. Once you addressed these requests I'll take it for a spin.
There was a problem hiding this comment.
Instead of duplicating the code for an SVG can you do something so it references the imported SVG? I think src="... somehow definition of the svg here instead of the path... " is possible.
There was a problem hiding this comment.
Sadly wasn't, left the comment in the SVG
| import { theme } from '../ui/GlobalStyle' | ||
|
|
||
| // Shared marker + label rendering used by both the map (LocationMarkers) and | ||
| // the Street View panorama (PanoramaHandler), so both look identical. |
There was a problem hiding this comment.
Could you remove all LLM comments from your change, and all comments that explain what the code does? Comments that explain why the code does something can stay if hard-won, like if we needed to fix a bug caused by a gotcha.
| } | ||
|
|
||
| const getLabelStyleConfig = (mapType) => { | ||
| const configs = { |
There was a problem hiding this comment.
Could this be an if statement?
There was a problem hiding this comment.
Can you pick a name without "helpers" that corresponds to something on the site?
| anchor: new googleMaps.Point(24, 44), | ||
| }) | ||
|
|
||
| // Dedupe the nearby locations by id (the viewed location may also appear in |
There was a problem hiding this comment.
That's not right, the selected location will definitely be in the map locations list - see redux. You can just use the selected location to draw the pin and the other locations for dots.
| constructor(googleMap, googleMaps, options) { | ||
| this.googleMap = googleMap | ||
| this.googleMaps = googleMaps | ||
| this.centerLocation = options.centerLocation |
There was a problem hiding this comment.
Can you check how I call this concept elsewhere in the code? I think I went for 'selected location'.
| // Always use the satellite/hybrid label style (white text, dark | ||
| // outline) since the panorama is imagery — more readable than the | ||
| // road-map style regardless of the map's current type. | ||
| const label = createLabel( |
There was a problem hiding this comment.
Change the interface of createLabel so it accomodates this concept rather than passing MapType.Hybrid here.
There was a problem hiding this comment.
Actually since createLabel just has a big switch statement inside, we could have createWhiteTextLabel and createBlackTextLabel, call always the white one in PanoramaHandler, and have a conditional on what to call inside LocationMarkers
| radius: 50, | ||
| }) | ||
|
|
||
| // disconnect() may have run while we awaited getPanorama (a newer |
There was a problem hiding this comment.
Not sure how would this happen from interactions in the UI
|
Thanks! I thought I can do the cleanup for the markers code, I'll take it from here, see #1142. |
3 participants
closes #1081