HI-FI: Update Expanded Route Timeline Badges#160
Conversation
|
Thanks for the timeline redesign — the cyan badges + dotted connector pattern looks clean and the One bug to fix before merge: Two minor nits:
The rest looks good — |
d5f6c00 to
9a22c48
Compare
b23b7a0 to
45daa2a
Compare
kirillakovalenko
left a comment
There was a problem hiding this comment.
PR #160 — HI-FI: Update Expanded Route Timeline Badges
Author: KesarSidhu | +397 / -198 across 7 files | Status: OPEN
Overview
This PR redesigns the results page sidebar with a hi-fi timeline layout: cyan sequence badges with dotted connectors between stops, a new route card design with an icon, a revamped header with a shared nav sidebar, and custom SVG map pin markers per route color. It also upgrades SidebarResultsButton to have active/inactive/disabled states based on sessionStorage.
Bugs
- "Save" pin-move button is always rendered — page.tsx:722-729
The "Cancel" button is correctly gated behind pendingPinMove != null, but the new "Save" button sits outside that condition and is always visible. Clicking it when nothing is pending will call savePendingPinMove with null state.
- {pendingPinMove != null && (
Cancel - )}
- Save ← always visible
- {pendingPinMove != null && (
- <>
-
<button onClick={cancelPendingPinMove}>Cancel</button> -
<button onClick={savePendingPinMove}>Save</button> - </>
- )}
- Advanced marker anchor offset is wrong — Map.tsx:163-164
The comment says "shift so the SVG tip sits on the stop," but translate(-50%, -50%) places the center of the 28×40 wrapper at the coordinate, not the bottom tip. For a pin shape, it should be translate(-50%, -100%) to anchor the tip at the lat/lng point.
Non-functional / Incomplete Code
3. Export button has no handler — page.tsx:730-735
- isSidebarOpen hardcoded to true — page.tsx:652
const isSidebarOpen = true; // was: useState(true)
The toggle button was removed, which is fine if the sidebar is always open. But leaving the variable (used only to set a CSS class) is dead code. Remove the isSidebarOpen variable and inline "w-[28rem]" directly, or document why the toggle was removed.
Design/Architecture Observations
5. SidebarResultsButton reads sessionStorage non-reactively — SidebarResultsButton.tsx:42
readHasOptimizeResults() is called directly in the render body. The value won't update if sessionStorage changes after the component mounts. This is acceptable only if the component always unmounts/remounts after optimization (e.g., full navigation). Worth confirming.
- hasOptimizeResults.ts casts without validation — hasOptimizeResults.ts:100
const parsed = JSON.parse(stored) as Route[];
return Array.isArray(parsed) && parsed.length > 0;
The as Route[] cast is fine for a boolean check since only Array.isArray and .length are used. No issue in practice, but worth noting for future callers that this doesn't validate the shape.
- Timeline connector min-h-[20px] may clip on tall stop cards — Sidebar.tsx:576-578
The connector uses flex-1 min-h-[20px]. If a stop card's content is shorter than the connector wants to be, it could appear detached. Verified visually before merge.
- Sidebar width jump: w-72 → w-[28rem] (288 → 448px)
The sidebar grew by ~56% in width. On 1280px-wide viewports this leaves ~780px for the map. Worth confirming this doesn't break layout at common screen widths.
Positive Notes
hasOptimizeResults.ts correctly guards against SSR with typeof window === "undefined".
Replacing the disabled with a in SidebarResultsButton is the correct accessibility pattern.
fill="currentColor" on SVG icons instead of hardcoded CSS vars is a good cleanup.
The module-level markerScaledSize lazy singleton in Map.tsx is a sensible optimization for the Google Maps Size object.
Edit mode banner moved from sidebar to a map overlay tooltip — cleaner UX.
Validation Gaps
The PR checklist has several unchecked items: format:check, npm run test, npm run build, all mobile checks, and all backend checks. The bug in item 1 above would be caught by a smoke test — recommend running at minimum lint, typecheck, and build before merge.
Summary
Two clear bugs to fix before merge: the always-visible "Save" button (#1) and the map pin anchor offset (#2). The Export button stub (#3) and hardcoded isSidebarOpen (#4) are polish issues. The timeline layout itself looks well-structured.
markboenigk
left a comment
There was a problem hiding this comment.
The duplicate sequence badge is removed from EditableStopItem — condition met. Both bugs flagged in kirillakovalenko's review are non-issues in the current code (Save button is correctly gated, pin anchor is already translate(-50%, -100%)). CI is all green. Approved.
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>
Co-authored-by: Cursor <cursoragent@cursor.com>
Refine the sidebar title section, edit action, and footer copy styling to better match the hifi mock while keeping the route list behavior unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Use route palette colors for map pins and switch marker visuals from plain dots to pin-style markers so map markers align with route styling. Co-authored-by: Cursor <cursoragent@cursor.com>
Match the edit-mode layout with a wider results panel, save-edits CTA styling, and centered map status banner while keeping existing route behavior intact. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add cyan timeline markers with dotted connectors in expanded routes. Remove duplicate sequence badge from EditableStopItem. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
9d416f4 to
cf0e2d1
Compare
Summary
Expanded route cards on the results sidebar use a hi-fi timeline layout: cyan sequence badges, dotted connectors between stops, and stop cards that show the sequence only once.
Motivation
The previous expanded stop list used a flat stack with a duplicate sequence badge inside each card. The hi-fi design calls for a vertical timeline so drivers can scan stop order at a glance.
Changes
app/ui/src/app/results/components/Sidebar.tsxisLastStopomits the trailing line).w-0+border-l-2.app/ui/src/app/results/components/EditableStopItem.tsxValidation
Frontend
npm --prefix app/ui run lintnpm --prefix app/ui run typechecknpm --prefix app/ui run format:checknpm --prefix app/ui run testnpm --prefix app/ui run buildnpm --prefix app/mobile run lintnpm --prefix app/mobile run typecheckBackend
cmake --preset dev.github/scripts/check-backend-static.sh build/devcmake --build --preset dev --parallelctest --preset dev --output-on-failure --no-tests=error -LE 'e2e|docker'docker compose -f deploy/compose/docker-compose.arm64.yml --env-file deploy/env/http-server.arm64.env configManually verified expanded routes on
/results: one sequence badge per stop, connectors between stops, no duplicate numbers in cards.Risk
Low — UI-only; no API, routing, or sessionStorage behavior changes.
Rollout and Recovery
Ship with the results page. Revert this PR if timeline spacing or badge styling needs adjustment.