HI-FI: Route Colors for Results Map Pins#152
Conversation
|
Same branch-strategy note as #149 and #150 — this PR shares 5+ files with both of those and all three target New code specific to this PR The custom pin markers are a nice touch, but two issues:
Carried over from #149/#150 (still unresolved)
Nit: |
ac4ca9a to
161ed49
Compare
Align the results header with the mock by using a compact app label plus Save/Export actions while preserving the existing sidebar toggle and pending-edit cancel behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Show Save/Cancel only when a pin move is pending, disable Export until export modal ships, and restore a screen-reader Results heading. Co-authored-by: Cursor <cursoragent@cursor.com>
Replay the full pre-rebase results stack (route colors, map polylines, top bar, left strip, routes panel) on current upstream/main in one commit to avoid conflicts with directions work already merged upstream. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid setState in useEffect (react-hooks/set-state-in-effect) by reading sessionStorage when rendering SidebarResultsButton. Co-authored-by: Cursor <cursoragent@cursor.com>
Use SIDEBAR_NAV_ITEM_INACTIVE (exported from formStyles.v2) and restore optional phoneNumber on Stop for vroomToRoutes. Co-authored-by: Cursor <cursoragent@cursor.com>
Advanced and fallback markers share the same SVG icon, tip-aligned coordinates, and a module-level scaledSize singleton. Co-authored-by: Cursor <cursoragent@cursor.com>
Point results page and SidebarResultsButton at layout/sidebar and edit/utils after folder restructure on upstream/main. Co-authored-by: Cursor <cursoragent@cursor.com>
161ed49 to
646f0a9
Compare
kirillakovalenko
left a comment
There was a problem hiding this comment.
Here is my review of PR #152 — HI-FI: Route Colors for Results Map Pins.
Overview
This PR introduces route-colored map pins (teardrop SVG) for both the Advanced Markers and classic Marker fallback paths, and makes several surrounding UI changes: a refactored results sidebar, a shared edit nav sidebar on the results page, and smart SidebarResultsButton state (active / navigable / disabled).
Scope is broader than the title suggests. In addition to map pins, the PR restructures the results page header, results sidebar layout, and adds the edit-page's sidebar to the results page. This scope creep is worth noting.
Bugs / Correctness
1. Advanced Marker tip anchor is wrong
Map.tsx
wrapper.style.transform = "translate(-50%, -50%)";
The teardrop SVG has its tip at the bottom of the image (y=40 in a 0 0 28 40 viewBox). translate(-50%, -50%) places the center of the wrapper at the stop coordinate, not the tip. The result is the pin floats ~20 px above the actual stop.
For the tip to sit on the stop, you need the bottom-center of the image to land on the coordinate:
wrapper.style.transform = "translate(-50%, -100%)";
The classic Marker fallback gets this right with anchor: new google.maps.Point(MARKER_ICON_WIDTH / 2, MARKER_ICON_HEIGHT).
2. Hydration mismatch in SidebarResultsButton
SidebarResultsButton.tsx
readHasOptimizeResults() reads sessionStorage synchronously during render. On the server it returns false (SSR guard), so the component renders as disabled, but on the client it may render as a link. Next.js will warn about (or silently mis-hydrate) this.
Fix with the standard pattern:
const [hasStoredRoutes, setHasStoredRoutes] = useState(false);
useEffect(() => { setHasStoredRoutes(readHasOptimizeResults()); }, []);
3. Save and Cancel buttons look identical
page.tsx
The pending-pin-move Save button was previously styled bg-amber-500 (clearly the primary action). Now both Save and Cancel share rounded-full border border-zinc-300 bg-white — visually indistinguishable. Users may click Cancel when intending to Save.
Design / UX Issues
4. Hardcoded branding footer in Sidebar
results/components/Sidebar.tsx
<p className="text-3xl leading-none font-semibold text-[var(--edit-teal-500)]">b²</p>
<p>Built with ❤️ for Humanity.</p>
<p>The Benevolent Bandwidth Foundation</p>
This looks like placeholder/demo content that will be awkward in production. If this is intentional branding, it should be in a shared constant or config, not inline JSX in a sidebar component.
5. Visually hidden <h1>
page.tsx
<h1 className="sr-only">Results</h1>
The visible heading is replaced by the brand label "DELIVERY OPTIMIZER". This is an accessibility tradeoff — screen readers still get an h1, but sighted users lose page-title context.
Code Quality
6. Results page imports edit-page internals
page.tsx
import styles from "../edit/edit.module.css";
import EditSidebar from "../edit/components/layout/sidebar/Sidebar";
The results page now reaches into the edit page's module-level CSS and sidebar component. These components are not abstracted into a shared layout. If the edit page's sidebar changes, the results page breaks. Consider extracting the shared nav sidebar into app/ui/src/app/components/ or a layout/ subtree.
7. Weak type validation in hasOptimizeResults.ts
hasOptimizeResults.ts
const parsed = JSON.parse(stored) as Route[];
return Array.isArray(parsed) && parsed.length > 0;
The as Route[] cast is unchecked — only array length is validated, not the Route shape. Stale/corrupted sessionStorage with a non-empty array of wrong shape would still return true and show the Results button as navigable. At minimum validate one required field (e.g., vehicleId).
8. Module-level mutable state
Map.tsx
let markerScaledSize: google.maps.Size | undefined;
Module-level mutable state leaks across tests and could cause issues in future SSR/RSC environments. The lazy init pattern is fine in production, but a factory function returning a local value (or useRef if it were a hook) would be cleaner.
Validation Gaps
The PR checklist shows npm run test and npm run build unchecked. Given the hydration change and advanced-marker anchor issue, running both before merge is important.
Summary
| -- | -- Must fix before merge | Advanced marker tip anchor (#1), Save/Cancel button parity (#3) Should fix before merge | Hydration mismatch (#2), edit-page coupling (#6) Low priority / follow-up | Branding footer (#4), hidden h1 (#5), weak type validation (#7) Tests | Uncheck boxes must be addressed — build in particular can reveal type errors from the new imports Here is my review of PR #152 — HI-FI: Route Colors for Results Map Pins.Overview
This PR introduces route-colored map pins (teardrop SVG) for both the Advanced Markers and classic Marker fallback paths, and makes several surrounding UI changes: a refactored results sidebar, a shared edit nav sidebar on the results page, and smart SidebarResultsButton state (active / navigable / disabled).
Scope is broader than the title suggests. In addition to map pins, the PR restructures the results page header, results sidebar layout, and adds the edit-page's sidebar to the results page. This scope creep is worth noting.
Bugs / Correctness
- Advanced Marker tip anchor is wrong
Map.tsx
wrapper.style.transform = "translate(-50%, -50%)";
The teardrop SVG has its tip at the bottom of the image (y=40 in a 0 0 28 40 viewBox). translate(-50%, -50%) places the center of the wrapper at the stop coordinate, not the tip. The result is the pin floats ~20 px above the actual stop.
For the tip to sit on the stop, you need the bottom-center of the image to land on the coordinate:
wrapper.style.transform = "translate(-50%, -100%)";
The classic Marker fallback gets this right with anchor: new google.maps.Point(MARKER_ICON_WIDTH / 2, MARKER_ICON_HEIGHT).
- Hydration mismatch in SidebarResultsButton
SidebarResultsButton.tsx
readHasOptimizeResults() reads sessionStorage synchronously during render. On the server it returns false (SSR guard), so the component renders as disabled, but on the client it may render as a link. Next.js will warn about (or silently mis-hydrate) this.
Fix with the standard pattern:
const [hasStoredRoutes, setHasStoredRoutes] = useState(false);
useEffect(() => { setHasStoredRoutes(readHasOptimizeResults()); }, []);
3. Save and Cancel buttons look identical
page.tsx
The pending-pin-move Save button was previously styled bg-amber-500 (clearly the primary action). Now both Save and Cancel share rounded-full border border-zinc-300 bg-white — visually indistinguishable. Users may click Cancel when intending to Save.
Design / UX Issues
4. Hardcoded branding footer in Sidebar
results/components/Sidebar.tsx
b²
Built with ❤️ for Humanity.
The Benevolent Bandwidth Foundation
This looks like placeholder/demo content that will be awkward in production. If this is intentional branding, it should be in a shared constant or config, not inline JSX in a sidebar component.- Visually hidden
page.tsx
Results
The visible heading is replaced by the brand label "DELIVERY OPTIMIZER". This is an accessibility tradeoff — screen readers still get an h1, but sighted users lose page-title context.Code Quality
6. Results page imports edit-page internals
page.tsx
import styles from "../edit/edit.module.css";
import EditSidebar from "../edit/components/layout/sidebar/Sidebar";
The results page now reaches into the edit page's module-level CSS and sidebar component. These components are not abstracted into a shared layout. If the edit page's sidebar changes, the results page breaks. Consider extracting the shared nav sidebar into app/ui/src/app/components/ or a layout/ subtree.
- Weak type validation in hasOptimizeResults.ts
hasOptimizeResults.ts
const parsed = JSON.parse(stored) as Route[];
return Array.isArray(parsed) && parsed.length > 0;
The as Route[] cast is unchecked — only array length is validated, not the Route shape. Stale/corrupted sessionStorage with a non-empty array of wrong shape would still return true and show the Results button as navigable. At minimum validate one required field (e.g., vehicleId).
- Module-level mutable state
Map.tsx
let markerScaledSize: google.maps.Size | undefined;
Module-level mutable state leaks across tests and could cause issues in future SSR/RSC environments. The lazy init pattern is fine in production, but a factory function returning a local value (or useRef if it were a hook) would be cleaner.
Validation Gaps
The PR checklist shows npm run test and npm run build unchecked. Given the hydration change and advanced-marker anchor issue, running both before merge is important.
Summary
Must fix before merge Advanced marker tip anchor (#1), Save/Cancel button parity (#3)
Should fix before merge Hydration mismatch (#2), edit-page coupling (#6)
Low priority / follow-up Branding footer (#4), hidden h1 (#5), weak type validation (#7)
Tests Uncheck boxes must be addressed — build in particular can reveal type errors from the new imports
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Good fixes since the last round — the pin anchor, SVG unification, Save/Cancel styling, and top bar conditional rendering are all sorted. Two items before this is mergeable:
Once those are done this is ready to go. |
Summary
Makes map pins route-colored and updates marker visuals to match the design: each stop uses its route palette color with a consistent teardrop pin on Advanced and fallback markers.
Motivation
Map stops should match sidebar/route polyline colors so drivers can tie pins to routes at a glance. This PR stacks on #150 (
step2-routes-panel) and only changes marker rendering inMap.tsx.Changes
Frontend
app/ui/src/app/results/components/Map.tsx: Route-colored markers viarouteColorHex; shared SVG teardrop for Advanced Markers and classicMarkerfallback; tip anchored on the stop lat/lng; module-levelgoogle.maps.Sizefor fallback icons.Validation
Frontend
npm --prefix app/ui run lintnpm --prefix app/ui run format:checknpm --prefix app/ui run typechecknpm --prefix app/ui run testnpm --prefix app/ui run buildManual: On
/resultswith multiple routes, confirm pin colors match route cards/polylines; with and withoutNEXT_PUBLIC_GOOGLE_MAPS_MAP_ID, pins look the same shape and sit on the stop.Risk
Low. UI-only marker changes in
Map.tsx. No API or data model changes beyond what #150 already includes.Rollout and Recovery
Ship after #150 merges (or with base
step2-routes-panel). Revert this PR to restore default map markers.