Improving data visualisation options by making selects more accessible#388
Improving data visualisation options by making selects more accessible#388Arbyhisenaj wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the map “data visualisation” controls to make selection UIs more accessible and consistent by moving key selections into the legend and enhancing the shared Combobox trigger styling.
Changes:
- Extend
Comboboxto support trigger styling/size so it can visually align withSelect. - Move data source + column selection into
Legend, and simplifyVisualisationPanelto focus on style settings. - Simplify boundaries legend wrapper components and adjust settings entry point in
BoundariesControl.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shadcn/ui/combobox.tsx | Adds trigger customisation (triggerClassName, size) to support smaller/Select-like triggers. |
| src/app/(private)/map/[id]/components/controls/VisualisationPanel/VisualisationPanel.tsx | Removes data source/primary column selection from the panel; keeps style controls and secondary column/aggregation. |
| src/app/(private)/map/[id]/components/controls/BoundariesControl/LegendControl.tsx | Simplifies wrapper to render Legend directly. |
| src/app/(private)/map/[id]/components/controls/BoundariesControl/BoundariesControl.tsx | Shows settings icon only when a data source exists; always renders legend content when expanded. |
| src/app/(private)/map/[id]/components/Legend.tsx | Rebuilds legend to include data source + column selection UI, bivariate toggle, and boundaries selection. |
| src/app/(private)/map/[id]/components/DataSourceSelectButton.tsx | Exports DataSourceSelectModal for reuse from Legend. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface ComboboxProps { | ||
| options: ComboboxOption[]; | ||
| value: string; | ||
| onValueChange: (value: string) => void; | ||
| placeholder?: string; | ||
| searchPlaceholder?: string; | ||
| emptyMessage?: string; | ||
| /** Merged onto the trigger button (e.g. typography to match Select) */ | ||
| triggerClassName?: string; | ||
| size?: React.ComponentProps<typeof Button>["size"]; | ||
| } |
There was a problem hiding this comment.
Combobox is used with <Label htmlFor=...> elsewhere, but it doesn’t currently expose an id (or aria-labelledby) that gets forwarded to the trigger <Button>. This means labels can’t be programmatically associated with the combobox trigger for screen readers. Consider adding an optional id prop (and/or ariaLabel/ariaLabelledby) to ComboboxProps and forwarding it onto the trigger button.
| htmlFor="choropleth-column-2-select" | ||
| className="text-sm text-muted-foreground font-normal" | ||
| > | ||
| Column 2 | ||
| </Label> | ||
| <div className="flex items-center gap-2"> | ||
| <Combobox |
There was a problem hiding this comment.
Label uses htmlFor="choropleth-column-2-select", but the Combobox trigger does not receive that id (Combobox doesn’t accept/forward it). As a result, the label isn’t associated with the control for assistive tech. Either add an id/aria-labelledby to Combobox and pass it here, or replace htmlFor usage with an explicit aria-label/aria-labelledby on the combobox trigger.
| htmlFor="choropleth-column-2-select" | |
| className="text-sm text-muted-foreground font-normal" | |
| > | |
| Column 2 | |
| </Label> | |
| <div className="flex items-center gap-2"> | |
| <Combobox | |
| id="choropleth-column-2-label" | |
| className="text-sm text-muted-foreground font-normal" | |
| > | |
| Column 2 | |
| </Label> | |
| <div className="flex items-center gap-2"> | |
| <Combobox | |
| aria-labelledby="choropleth-column-2-label" |
| }} | ||
| > | ||
| <X className="h-3 w-3 mr-1" /> | ||
| Remove bivariate visualization |
There was a problem hiding this comment.
The user-facing text uses American spelling “visualization”, but this area of the app consistently uses “visualisation” (e.g. layer label “Data visualisation”). Please change this string to “visualisation” for consistency.
| Remove bivariate visualization | |
| Remove bivariate visualisation |
| ? "Remove second column" | ||
| : bivariatePickerOpen | ||
| ? "Cancel second column" | ||
| : "Add second column for bivariate visualization" |
There was a problem hiding this comment.
The aria-label text uses American spelling “visualization”, but elsewhere in this UI the project uses “visualisation” (e.g. “Data visualisation”). Please change this string to “visualisation” for consistency.
| : "Add second column for bivariate visualization" | |
| : "Add second column for bivariate visualisation" |
joaquimds
force-pushed
the
ux/Data-visualisation-upgrade
branch
from
a254031 to
4e57c1a
Compare
|
@Arbyhisenaj It's good, but it removes the column description popover and column metadata edit button: 
Which isn't the best design, but it's a feature we can't remove as it's required by AB Charitable Trust. Also smaller comments:
|
Description
Motivation and Context
How Can It Be Tested?
How Will This Be Deployed?
Screenshots (if appropriate):
Types of changes
Checklist: