fix(split-view): use real checkboxes in the secondary pane layer menu (#840)#865
Conversation
…#840) The per-pane layer-visibility dropdown drew only a bare checkmark for selected layers, which broke the checkbox design language used elsewhere for layer selection. Add an opt-in box-style indicator to DropdownMenuCheckboxItem and use it in the pane menu so every layer shows a real checkbox (empty when off, filled with a check when on). The default checkmark style is unchanged for the menu's other callers.
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe shared dropdown checkbox component now supports a box-style indicator variant. The split-view layer toggle uses that variant for its layer visibility items. ChangesCheckbox indicator variant
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: one or more packages not found in the registry. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Cloudflare Pages preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewThis is a small, well-scoped UI change. The core approach is sound: adding an Two findings worth addressing: CLAUDE.md (Medium confidence)Multi-line comment block on Quality / Edge case (Medium confidence)
Bugs — none foundSecurity — none foundPerformance — none foundWhat I checked: the full diff; surrounding context in |
- Condense the `indicator` prop's JSDoc from a 4-line block to a single line. - Handle the `"indeterminate"` checked state in the box indicator (a half-filled box with a dash) so the public API no longer conflates it with `false`.
| {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> |
There was a problem hiding this comment.
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):
- Guard against uncontrolled usage in the JSDoc / TypeScript so callers know
indicator="box"requires a controlledchecked:
| {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> |
- Or add a checked-state ref/state and sync it with Radix's
onCheckedChangeto 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.
| 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. */ |
There was a problem hiding this comment.
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:
| /** `"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`). */ |
Code reviewReviewed the two changed files ( Bugs
Quality
Security / Performance / CLAUDE.mdNothing to flag. The change is purely UI/visual, adds no external I/O, and no direct-to-main commit. Backward compatibility with all existing callers ( |
1 participant
Summary
Addresses the "real checkboxes for map views" request in #840 (Issue 1).
In a split view, each secondary map pane has a Layer visibility dropdown so it can show a different subset of the shared layers. That dropdown drew only a bare checkmark next to selected layers, with nothing in front of unselected ones. As reported, this breaks the checkbox design language the rest of the app uses for layer selection.
This PR makes every layer in that menu show a real checkbox: an empty bordered box when the layer is off, a filled box with a checkmark when it is on.
Changes
packages/ui—DropdownMenuCheckboxItemgains an optionalindicator="box"prop that renders an always-visible checkbox box (border when unchecked, filled + check when checked). The default ("check") is unchanged, so the menu's other callers (View menu, Settings, Attribute Table) keep the existing checkmark style.apps/geolibre-desktop— the per-pane layer-visibility menu inMapGrid.tsxopts intoindicator="box".Scope
Limited to the checkbox UI (Issue 1). The split-view layer-stack-order and independent top-pane controls raised elsewhere in the issue are separate and not in scope here.
Verification
Drove the real app in a browser (two XYZ layers, two-column split view) and confirmed in both light and dark themes that each layer in the pane menu now renders a real checkbox, toggling between empty box and filled-with-check.
Fixes #840
Summary by CodeRabbit
New Features
indicatoroption for checkbox items in dropdown menus, supporting a boxed indicator style.Bug Fixes