Skip to content

Conversation

@Arbyhisenaj
Copy link
Member

-add/remove layers

  • consolidated members and markers
  • updated icons
  • Custom colours for categories
  • Empty state button for adding layers
  • more legible legend when showing a long list of categories (e.g parties with the voteshare data)

Description

Motivation and Context

How Can It Be Tested?

How Will This Be Deployed?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I've checked the spec (e.g. Figma file) and documented any divergences.
  • My code follows the code style of this project.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I've updated the documentation accordingly.
  • Replace unused checkboxes with bullet points.

{/* Potentially could be restored. Remove if still not restored by 2025-03-01 */}
{/* <ShapeSelector />
<Separator />
<FillSelector /> */}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: remove this

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the layers panel with comprehensive updates to improve user experience when managing map layers, markers, and data visualizations.

Changes:

  • Consolidated members and markers into a unified markers control panel
  • Added custom color picker for categorical data visualization
  • Implemented context menus for renaming, hiding, and deleting layers/markers/turfs/data sources
  • Replaced hover menus with right-click context menus for better UX
  • Added empty state buttons for adding layers with dropdown options
  • Improved legend display for categorical data with color swatches
  • Added column filtering UI for "highest value" visualizations
  • Updated icon library (lucide-react) and replaced custom VectorSquare icon

Reviewed changes

Copilot reviewed 33 out of 34 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
src/server/trpc/routers/dataSource.ts Added name field to update and conditional import triggering
src/server/stats/index.ts Added includeColumns parameter with column filtering logic and float casting
src/server/models/MapView.ts Added categoryColors and includeColumnsString schema fields
src/components/DeleteConfirmationDialog.tsx New reusable delete confirmation dialog component
src/app/map/[id]/hooks/useLayers.ts Added marker visibility handling logic
src/app/map/[id]/components/controls/MarkersControl/* Refactored to use context menus and added delete confirmations
src/app/map/[id]/components/controls/TurfsControl/* Replaced hover menus with context menus
src/app/map/[id]/components/controls/DataSourceItem.tsx Added inline renaming and context menu actions
src/app/map/[id]/components/controls/LayerEmptyMessage.tsx Enhanced with button mode and dropdown support
src/app/map/[id]/components/controls/VisualisationPanel/* Added IncludeColumnsModal and CategoryColorEditor
src/app/map/[id]/components/Legend.tsx Redesigned with category swatches and visibility toggle
src/app/map/[id]/colors.ts Added categoryColors support for custom categorical colors
src/components/icons/VectorSquare.tsx Removed custom icon (replaced with lucide-react)
package.json Updated lucide-react from 0.484.0 to 0.562.0

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 156 to 167
.filter(({ name, type }) => {
// Must be a number column
if (type !== ColumnType.Number) return false;

// If includeColumns is specified, only include those columns
if (includeColumns && includeColumns.length > 0) {
return includeColumns.includes(name);
}

// Otherwise, exclude columns in excludeColumns list
return !excludeColumns.includes(name);
})
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When includeColumns is specified with length > 0, the excludeColumns list is completely ignored. This could lead to unexpected behavior if a user expects both include and exclude to work together. Consider either documenting this behavior clearly or handling the case where both are provided (e.g., by applying excludeColumns as a filter after includeColumns).

Copilot uses AI. Check for mistakes.
"kysely": "^0.27.6",
"lucide": "^0.544.0",
"lucide-react": "^0.484.0",
"lucide-react": "^0.562.0",
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lucide-react package is being updated from version 0.484.0 to 0.562.0. This is a significant jump that may introduce breaking changes or new icons (like VectorSquareIcon which is being used in this PR). Ensure that all icon imports are compatible with the new version and check the lucide-react changelog for any breaking changes.

Copilot uses AI. Check for mistakes.
Comment on lines 250 to 251
typeof input.autoEnrich === "boolean" ||
typeof input.autoImport === "boolean";
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition for checking if autoEnrich and autoImport have changed uses typeof to check if they're boolean, but this doesn't verify if the values actually changed from their previous state. This means any update that includes these fields (even if unchanged) will trigger a re-import. Consider comparing against the previous values to only trigger import when there's an actual change.

Copilot uses AI. Check for mistakes.
Comment on lines +96 to +99
<p className="text-sm text-gray-500 mb-4">
Only selected columns will be considered when determining the
highest value column for each area. Leave empty to use all numeric
columns.
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IncludeColumnsModal description states "Leave empty to use all numeric columns", but this conflicts with the actual behavior where an empty includeColumns results in using all columns except those in excludeColumns. The UI text should be updated to clarify that when no columns are selected, the excludeColumns filter is applied instead.

Copilot uses AI. Check for mistakes.
Comment on lines +190 to +194
enableVisibilityToggle={Boolean(
placedMarkers.length > 0 ||
markerDataSources.length > 0 ||
membersDataSource,
)}
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition checking if the layer should be enabled uses Boolean(placedMarkers.length > 0 || ...) which is redundant since the expression already evaluates to a boolean. The outer Boolean() call is unnecessary and can be removed for cleaner code.

Suggested change
enableVisibilityToggle={Boolean(
placedMarkers.length > 0 ||
markerDataSources.length > 0 ||
membersDataSource,
)}
enableVisibilityToggle={
placedMarkers.length > 0 ||
markerDataSources.length > 0 ||
membersDataSource
}

Copilot uses AI. Check for mistakes.
>
{/* Fill Layer - only show for choropleth */}
<Layer
key={`${sourceId}-fill-${viewConfig.areaDataColumn}-${viewConfig.calculationType}-${viewConfig.colorScheme}`}
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Layer key includes categoryColors in its dependencies via viewConfig.categoryColors, but categoryColors is an object/record. Since the key is a string template, changes to categoryColors won't be reflected in the key and the layer won't re-render when category colors change. Consider adding a serialized version of categoryColors to the key or using a different approach to force layer updates when colors change.

Copilot uses AI. Check for mistakes.
if (dropdownItems) {
return (
<MultiDropdownMenu
dropdownLabel="Marker options"
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dropdownLabel is hardcoded to "Marker options" in the AddLayerButton component, but this component is used for different layer types (turfs, boundaries, markers). This makes the dropdown label incorrect when used with non-marker layers. Consider making dropdownLabel a parameter or deriving it from the context.

Copilot uses AI. Check for mistakes.
Comment on lines 244 to 251
const configFieldsChanged =
input.columnRoles !== undefined ||
input.enrichments !== undefined ||
input.geocodingConfig !== undefined ||
input.dateFormat !== undefined ||
input.public !== undefined ||
typeof input.autoEnrich === "boolean" ||
typeof input.autoImport === "boolean";
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition configFieldsChanged checks if fields are !== undefined, but this means passing undefined explicitly won't trigger a re-import, while passing null or any other value will. This could lead to confusing behavior. Consider using in operator to check if the property exists in the input object, or be more explicit about what constitutes a change.

Copilot uses AI. Check for mistakes.
typeof input.autoEnrich === "boolean" ||
typeof input.autoImport === "boolean";

if (configFieldsChanged) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: remove public, autoenrich, autoimport from the check here

column: z.string(),
secondaryColumn: z.string().optional(),
excludeColumns: z.array(z.string()),
includeColumns: z.array(z.string()).optional().nullable(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: remove excludeColumns

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 42 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +244 to +254
const configFieldsChanged =
input.columnRoles !== undefined ||
input.enrichments !== undefined ||
input.geocodingConfig !== undefined ||
input.dateFormat !== undefined;

if (configFieldsChanged) {
await enqueue("importDataSource", ctx.dataSource.id, {
dataSourceId: ctx.dataSource.id,
});
}
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic check for configFieldsChanged only checks if the fields are not undefined, but doesn't verify if they've actually changed from their previous values. This means that even if a user updates the name and then clicks save without changing config fields, but the config fields happen to be defined, the import will not be triggered. Consider comparing the new values with existing values to determine if a re-import is truly necessary.

Copilot uses AI. Check for mistakes.
Comment on lines +78 to +85
map.setFeatureState(
{
source: sourceId,
sourceLayer: layerId,
id: areaCode,
},
{ value: null, secondaryValue: null },
);
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function removeFeatureState has been changed to setFeatureState with null values. While this should work, verify that this is the intended behavior change. The comment says "Remove lingering feature states" but the implementation now sets them to null rather than removing them. Ensure this doesn't cause any unexpected behavior with Mapbox GL.

Copilot uses AI. Check for mistakes.
): AreaSetGroupCode[] => {
if (!dataSourceGeocodingConfig) {
return [];
return Object.values(AreaSetGroupCode);
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When no geocoding config is provided, the function now returns all AreaSetGroupCodes instead of an empty array. This is a significant behavior change that could allow users to select area sets that aren't compatible with their data source. This might lead to incorrect visualizations or errors when geocoding is attempted. Verify this change is intentional and that there are proper safeguards elsewhere in the code.

Suggested change
return Object.values(AreaSetGroupCode);
return [];

Copilot uses AI. Check for mistakes.
Comment on lines +104 to +105
className="flex items-center gap-2 w-full min-h-full p-1 rounded transition-colors hover:bg-neutral-100 text-left cursor-pointer"
onClick={() => handleFlyTo(turf)}
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The styling classes use forward slashes (/) as separators which is unusual and might not be valid Tailwind CSS syntax. For example: "flex items-center gap-2 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer". This should use spaces only. Verify if this is intentional or if these are typos that could cause styling issues.

Copilot uses AI. Check for mistakes.
ref={isDraggingMarker ? setHeaderNodeRef : null}
onClick={() => onClickFolder()}
className={cn(
"flex items-center gap-1 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer",
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar issue with forward slashes in className strings that appear to be separators. These should be removed as they're not valid Tailwind CSS syntax: "flex items-center gap-1 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer". This pattern appears in multiple places and should be cleaned up.

Suggested change
"flex items-center gap-1 / w-full min-h-full p-1 rounded / transition-colors hover:bg-neutral-100 / text-left cursor-pointer",
"flex items-center gap-1 w-full min-h-full p-1 rounded transition-colors hover:bg-neutral-100 text-left cursor-pointer",

Copilot uses AI. Check for mistakes.
@@ -1,5 +1,4 @@
import { ChartBar, MapPin } from "lucide-react";
import VectorSquare from "@/components/icons/VectorSquare";
import { ChartBar, MapPin, VectorSquareIcon } from "lucide-react";
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VectorSquareIcon icon may not exist in lucide-react. The PR updates lucide-react to version 0.562.0, and while this version exists, VectorSquareIcon might not be the correct icon name. In the previous code, a custom VectorSquare component was used. Please verify that VectorSquareIcon is a valid icon in lucide-react 0.562.0, otherwise this will cause a runtime error.

Copilot uses AI. Check for mistakes.
MapPinIcon,
SquareIcon,
UsersIcon,
VectorSquareIcon,
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VectorSquareIcon may not exist in lucide-react. A similar issue exists here where the code references VectorSquareIcon from lucide-react. Please verify this is a valid icon name in version 0.562.0, or use the correct icon name to avoid runtime errors.

Copilot uses AI. Check for mistakes.
@joaquimds joaquimds merged commit 1d0920b into main Jan 19, 2026
7 checks passed
@joaquimds joaquimds deleted the layers-panel/jan26-updates-shorterm branch January 19, 2026 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants