Skip to content

fix(split-view): use real checkboxes in the secondary pane layer menu (#840) #865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
giswqs merged 2 commits into from
Jun 25, 2026
+30 −7
Merged

fix(split-view): use real checkboxes in the secondary pane layer menu (#840) #865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
42927ba
fix(split-view): use real checkboxes in the secondary pane layer menu…
giswqs Jun 25, 2026
fd42de3
Address Claude review feedback
giswqs Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/geolibre-desktop/src/components/layout/MapGrid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ function PaneLayerToggle({ viewId, index }: PaneLayerToggleProps) {
return (
<DropdownMenuCheckboxItem
key={layer.id}
indicator="box"
checked={visible}
onCheckedChange={(checked: boolean) =>
setSecondaryLayerVisibility(viewId, layer.id, checked)
Expand Down
36 changes: 29 additions & 7 deletions packages/ui/src/components/dropdown-menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,11 @@ DropdownMenuItem.displayName = DropdownMenuPrimitive.Item.displayName;

export const DropdownMenuCheckboxItem = React.forwardRef<
React.ElementRef<typeof DropdownMenuPrimitive.CheckboxItem>,
React.ComponentPropsWithoutRef<typeof DropdownMenuPrimitive.CheckboxItem>
>(({ className, children, checked, ...props }, ref) => (
React.ComponentPropsWithoutRef<typeof DropdownMenuPrimitive.CheckboxItem> & {
/** `"check"` (default): bare checkmark when checked. `"box"`: always-visible bordered square that fills on check. */

@github-actions github-actions Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit (quality): The JSDoc describes visual behaviour but doesn't flag the controlled-only constraint. Adding a note here makes the limitation discoverable at the call site:

Suggested change
/** `"check"` (default): bare checkmark when checked. `"box"`: always-visible bordered square that fills on check. */
/** `"check"` (default): bare checkmark when checked. `"box"`: always-visible bordered square that fills on check. `indicator="box"` requires a controlled `checked` prop (not `defaultChecked`). */

indicator?: "check" | "box";
}
>(({ className, children, checked, indicator = "check", ...props }, ref) => (
<DropdownMenuPrimitive.CheckboxItem
ref={ref}
className={cn(
Expand All @@ -125,11 +128,30 @@ export const DropdownMenuCheckboxItem = React.forwardRef<
checked={checked}
{...props}
>
<span className="absolute left-2 flex h-3.5 w-3.5 items-center justify-center">
<DropdownMenuPrimitive.ItemIndicator>
<Check className="h-4 w-4" />
</DropdownMenuPrimitive.ItemIndicator>
</span>
{indicator === "box" ? (
<span
className={cn(
"absolute left-2 flex h-4 w-4 items-center justify-center rounded-[4px] border transition-colors",
checked === true
? "border-primary bg-primary text-primary-foreground"
: checked === "indeterminate"
? "border-primary bg-primary/40 text-primary-foreground"
: "border-input",
)}
>
{checked === true ? (
<Check className="h-3 w-3" />
) : checked === "indeterminate" ? (
<span className="h-0.5 w-2 rounded-full bg-current" />
) : null}
</span>
Comment on lines +131 to +147

@github-actions github-actions Bot Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug (medium confidence): indicator="box" silently breaks in uncontrolled mode

The "box" branch reads visual state directly from the checked prop, bypassing DropdownMenuPrimitive.ItemIndicator. When a caller uses uncontrolled mode (passing defaultChecked instead of checked), checked will be undefined here and the box will always render empty—Radix manages the internal checked state but the visual never updates.

The "check" path avoids this because ItemIndicator is wired to Radix's internal state and works in both modes.

Mitigations (pick one):

  1. Guard against uncontrolled usage in the JSDoc / TypeScript so callers know indicator="box" requires a controlled checked:
Suggested change
{indicator === "box" ? (
<span
className={cn(
"absolute left-2 flex h-4 w-4 items-center justify-center rounded-[4px] border transition-colors",
checked === true
? "border-primary bg-primary text-primary-foreground"
: checked === "indeterminate"
? "border-primary bg-primary/40 text-primary-foreground"
: "border-input",
)}
>
{checked === true ? (
<Check className="h-3 w-3" />
) : checked === "indeterminate" ? (
<span className="h-0.5 w-2 rounded-full bg-current" />
) : null}
</span>
{indicator === "box" ? (
// NOTE: "box" mode reads `checked` directly and only works when `checked`
// is explicitly controlled (not with `defaultChecked`).
<span
aria-hidden="true"
className={cn(
"absolute left-2 flex h-4 w-4 items-center justify-center rounded-[4px] border transition-colors",
checked === true
? "border-primary bg-primary text-primary-foreground"
: checked === "indeterminate"
? "border-primary bg-primary/40 text-primary-foreground"
: "border-input",
)}
>
{checked === true ? (
<Check className="h-3 w-3" />
) : checked === "indeterminate" ? (
<span className="h-0.5 w-2 rounded-full bg-current" />
) : null}
</span>
  1. Or add a checked-state ref/state and sync it with Radix's onCheckedChange to handle uncontrolled mode (more involved).

The aria-hidden="true" addition in the suggestion above is also recommended regardless — the outer <span> is a decorative indicator and the parent menuitemcheckbox already carries the semantic state via aria-checked; leaving the span un-hidden can cause some screen readers to double-announce the item.

) : (
<span className="absolute left-2 flex h-3.5 w-3.5 items-center justify-center">
<DropdownMenuPrimitive.ItemIndicator>
<Check className="h-4 w-4" />
</DropdownMenuPrimitive.ItemIndicator>
</span>
)}
{children}
</DropdownMenuPrimitive.CheckboxItem>
));
Expand Down
Loading