Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces UI tweaks to the private map editor, expanding configuration options for secondary boundaries and adding a map layout (Geo/Hex) selector, alongside some related control-panel behaviour adjustments.
Changes:
- Add a configurable
secondaryBoundaryStrokeColortoMapViewconfig, expose it via a new colour picker in the legend, and apply it to the secondary boundaries Mapbox layer. - Add a “Layout” selector (Geo/Hex) in the visualisation panel and adjust markers/turfs availability in Hex mode.
- Minor UI/logic refinements (e.g. inspector boundary panel gating, small styling adjustments).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/models/MapView.ts | Extends map view config schema with secondaryBoundaryStrokeColor. |
| src/app/(private)/map/[id]/styles.ts | Adds a shared colour palette (mapColourPalette) for UI pickers. |
| src/app/(private)/map/[id]/components/SecondaryBoundaries.tsx | Uses view-configurable stroke colour and reduces line width. |
| src/app/(private)/map/[id]/components/Legend/Legend.tsx | Adds secondary boundaries UI (open/close + stroke colour picker + reset). |
| src/app/(private)/map/[id]/components/InspectorPanel/InspectorDataTab.tsx | Only shows “On map visualisation” block for boundaries when a choropleth visualisation exists. |
| src/app/(private)/map/[id]/components/controls/VisualisationPanel/VisualisationPanel.tsx | Adds layout (Geo/Hex) selector UI driven by mapTypes. |
| src/app/(private)/map/[id]/components/controls/TurfsControl/TurfsControl.tsx | Visually disables turfs control in Hex mode via wrapper class. |
| src/app/(private)/map/[id]/components/controls/MarkersControl/MarkersControl.tsx | Visually disables markers control in Hex mode via wrapper class. |
| src/app/(private)/map/[id]/components/controls/LayerControlWrapper.tsx | Adds className passthrough to support disabled styling. |
| src/app/(private)/map/[id]/components/controls/ControlPanel.tsx | Always renders markers/turfs controls (previously hidden for Hex). |
| src/app/(private)/map/[id]/components/BoundaryHoverInfo/AreasList.tsx | Small styling tweak to show a border around area colour swatches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/app/(private)/map/[id]/components/controls/TurfsControl/TurfsControl.tsx
Show resolved
Hide resolved
src/app/(private)/map/[id]/components/controls/MarkersControl/MarkersControl.tsx
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/app/(private)/map/[id]/styles.ts
Outdated
|
|
||
| export const DEFAULT_SECONDARY_BOUNDARY_STROKE_COLOR = "#555555"; | ||
|
|
||
| export const mapColourPalette: mapPaletteColor[] = [ |
There was a problem hiding this comment.
mapColourPalette introduces British spelling in an identifier while the surrounding codebase consistently uses Color in identifiers (e.g. mapColors, getCategoryColorScale, ColorScheme). Consider renaming this export (and related types) to mapColorPalette to avoid inconsistent naming and make it easier to discover via search/autocomplete.
| export const mapColourPalette: mapPaletteColor[] = [ | |
| export const mapColorPalette: mapPaletteColor[] = [ |
| const secondaryBoundaryStrokeColor = | ||
| viewConfig.secondaryBoundaryStrokeColor || "#555555"; | ||
| const secondaryBoundarySwatches = mapColourPalette.map((c) => c.color); |
There was a problem hiding this comment.
secondaryBoundaryStrokeColor falls back to a hard-coded "#555555" even though DEFAULT_SECONDARY_BOUNDARY_STROKE_COLOR is exported from styles.ts. Using the shared constant (and ?? instead of ||) would avoid duplication and ensure the fallback only triggers for null/undefined.
| <div className=" border-neutral-100 px-3 pb-3"> | ||
| {!secondaryBoundariesOpen && !viewConfig.secondaryAreaSetCode ? ( | ||
| <button | ||
| type="button" | ||
| className="text-xs text-muted-foreground underline cursor-pointer text-left hover:text-foreground transition-colors" |
There was a problem hiding this comment.
This container uses border-neutral-100 without a corresponding border-* class, so the color utility has no effect. If a separator is intended here, add an explicit border (e.g. border-t/border-b); otherwise remove the unused class for clarity.
| <div className="space-y-2"> | ||
| <p className="text-sm font-medium">Layout</p> | ||
| <div className="flex gap-2"> | ||
| {mapTypes.map((type) => { | ||
| const isDefault = !viewConfig.mapType && type === MapType.Geo; | ||
| const isChecked = viewConfig.mapType === type || isDefault; | ||
| return ( | ||
| <button | ||
| key={type} | ||
| type="button" | ||
| className={cn( | ||
| "flex-1 rounded-md border px-3 py-2 text-xs font-medium shadow-xs transition-colors", | ||
| "hover:bg-accent hover:text-accent-foreground hover:border-border", | ||
| isChecked && "border-blue-300", | ||
| )} | ||
| onClick={() => updateViewConfig({ mapType: type })} | ||
| > | ||
| {type === MapType.Hex ? "Hex map" : "Geographic"} |
There was a problem hiding this comment.
The new Layout toggle uses plain <button> elements with only a visual border to indicate selection. For accessibility, consider representing this as a proper radio/toggle group (e.g. role="radiogroup"/role="radio" or aria-pressed) so the selected state is conveyed to screen readers and keyboard users.
Description
Motivation and Context
How Can It Be Tested?
How Will This Be Deployed?
Screenshots (if appropriate):
Types of changes
Checklist: