|
| 1 | +--- |
| 2 | +name: explorer-sort-dropdown-auto-close |
| 3 | +issue: "" |
| 4 | +state: complete |
| 5 | +version: <next> |
| 6 | +date: 2026-05-06 |
| 7 | +git_commit: 4e006b32875d0a540867c949f59aa1d4200a4203 |
| 8 | +branch: main |
| 9 | +topic: "Auto-close the explorer sort dropdown when an item is clicked" |
| 10 | +tags: [plan, visualization, sidebar, explorer, daisyui, signals, bugfix] |
| 11 | +--- |
| 12 | + |
| 13 | +# Auto-close the explorer sort dropdown when an item is clicked |
| 14 | + |
| 15 | +## Goal |
| 16 | + |
| 17 | +Fix `ExplorerSortControlComponent` so the dropdown closes immediately after the user picks a sort key OR toggles ascending/descending. Today the menu stays open because the daisyUI dropdown is purely CSS-driven (`:focus-within`) and the option buttons remain inside the dropdown after click. Migrate the component to the same controlled-signal pattern already in use in `MapSelectorComponent` and `DeltaSelectorComponent`. |
| 18 | + |
| 19 | +## Current State Analysis |
| 20 | + |
| 21 | +`features/sidebarExplorer/components/explorerSortControl/explorerSortControl.component.html` uses daisyUI's pure-CSS dropdown: |
| 22 | + |
| 23 | +```html |
| 24 | +<div class="dropdown dropdown-start w-full px-2 mt-2"> |
| 25 | + <button type="button" class="btn ..."> ... </button> |
| 26 | + <ul class="dropdown-content menu ..."> |
| 27 | + @for (option of sortOptions; track option) { |
| 28 | + <li><button (click)="setSortingOption(option)">{{ option }}</button></li> |
| 29 | + } |
| 30 | + <li><button (click)="toggleSortOrder()"> ... </button></li> |
| 31 | + </ul> |
| 32 | +</div> |
| 33 | +``` |
| 34 | + |
| 35 | +daisyUI shows `.dropdown-content` while focus is inside `.dropdown`. The option buttons are still inside `.dropdown`, so clicking one keeps focus there → menu stays open. There is no JS state controlling visibility, no outside-click handler, no explicit close. |
| 36 | + |
| 37 | +The component is `OnPush` and reactive (`toSignal` of `sortState$`). It's already zoneless-compatible per the `features/` rule. |
| 38 | + |
| 39 | +## Desired End State |
| 40 | + |
| 41 | +- Clicking a sort-key option dispatches `setSortingOption(...)` **and** closes the dropdown. |
| 42 | +- Clicking the bottom "Sort ascending / Sort descending" toggle dispatches `toggleSortingOrderAscending()` **and** closes the dropdown. |
| 43 | +- Clicking the trigger button toggles the dropdown open/closed. |
| 44 | +- Clicking outside the component closes the dropdown. |
| 45 | +- All existing specs keep passing; new specs cover open / close / outside-click behaviour. |
| 46 | + |
| 47 | +## What We're NOT Doing |
| 48 | + |
| 49 | +- **Not refactoring `MapSelectorComponent` / `DeltaSelectorComponent`** — they already work correctly. This is a surgical fix scoped to the sort control. |
| 50 | +- **Not extracting a shared dropdown directive / component**. The pattern is short and repeated in only three places; abstraction can come later if a fourth appears. |
| 51 | +- **Not adding Escape-to-close, keyboard navigation, or ARIA improvements** beyond what already exists. Match the behaviour of the two reference dropdowns. |
| 52 | +- **Not animating the open/close.** Instant swap, same as today. |
| 53 | +- **Not changing the sort logic, store wiring, or `ExplorerSortService`.** |
| 54 | + |
| 55 | +## UI Mockups |
| 56 | + |
| 57 | +### Before — bug |
| 58 | + |
| 59 | +``` |
| 60 | +Sort: Name ▾ ↓ ← user clicks dropdown |
| 61 | + ┌────────────────────────────┐ |
| 62 | + │ Name │ ← user clicks "Number of files" |
| 63 | + │ Number of files │ ← dropdown STAYS OPEN |
| 64 | + │ Area Size │ |
| 65 | + │ ────────────────────────── │ |
| 66 | + │ ↑ Sort descending │ |
| 67 | + └────────────────────────────┘ |
| 68 | +``` |
| 69 | + |
| 70 | +### After — fix |
| 71 | + |
| 72 | +``` |
| 73 | +Sort: Name ▾ ↓ ← user clicks dropdown (open) |
| 74 | + ┌────────────────────────────┐ |
| 75 | + │ Name │ |
| 76 | + │ Number of files │ ← user clicks "Number of files" |
| 77 | + │ Area Size │ |
| 78 | + │ ────────────────────────── │ |
| 79 | + │ ↑ Sort descending │ |
| 80 | + └────────────────────────────┘ |
| 81 | +
|
| 82 | +Sort: Number of files ▾ ↓ ← dropdown CLOSED, label updated |
| 83 | +``` |
| 84 | + |
| 85 | +The visual layout of the dropdown contents is unchanged. |
| 86 | + |
| 87 | +## Architecture and Code Reuse |
| 88 | + |
| 89 | +### Pattern reused from `MapSelectorComponent` / `DeltaSelectorComponent` |
| 90 | + |
| 91 | +``` |
| 92 | +isOpen = signal(false) |
| 93 | +[class.dropdown-open]="isOpen()" on outer .dropdown |
| 94 | +@if (isOpen()) { <dropdown-content/> } ← optional but matches the two reference components |
| 95 | +@HostListener("document:click") ← close on outside click |
| 96 | +toggleOpen() / item-handlers .set(false) on dropdown close |
| 97 | +``` |
| 98 | + |
| 99 | +References: |
| 100 | +- `app/codeCharta/features/navBar/components/mapSelector/mapSelector.component.ts:25` (`isOpen = signal(false)`), `:54-72` (HostListener + toggle), `:132` (close on apply). |
| 101 | +- `app/codeCharta/features/navBar/components/deltaSelector/deltaSelector.component.ts:23,49-71` (same pattern with a tri-state signal). |
| 102 | + |
| 103 | +### Files affected |
| 104 | + |
| 105 | +- `app/codeCharta/features/sidebarExplorer/components/explorerSortControl/` |
| 106 | + - `explorerSortControl.component.ts` — **modified**: add `isOpen` signal, `toggleOpen()`, `HostListener("document:click")`, `ElementRef` injection; close in `setSortingOption` and `toggleSortOrder`. |
| 107 | + - `explorerSortControl.component.html` — **modified**: bind `[class.dropdown-open]` to the outer `.dropdown`, wire `(click)="toggleOpen()"` on the trigger button, wrap the `<ul class="dropdown-content ...">` in `@if (isOpen())`. |
| 108 | + - `explorerSortControl.component.spec.ts` — **modified**: add specs for open/close on trigger click, close after picking an option, close after toggling order, close on outside click. Existing tests stay green (the rendered DOM in the open state is unchanged). |
| 109 | + |
| 110 | +### Component sketch |
| 111 | + |
| 112 | +```ts |
| 113 | +// explorerSortControl.component.ts (key bits) |
| 114 | +import { ChangeDetectionStrategy, Component, ElementRef, HostListener, computed, inject, signal } from "@angular/core" |
| 115 | + |
| 116 | +export class ExplorerSortControlComponent { |
| 117 | + private readonly explorerSortService = inject(ExplorerSortService) |
| 118 | + private readonly elementRef = inject(ElementRef<HTMLElement>) |
| 119 | + |
| 120 | + readonly isOpen = signal(false) |
| 121 | + readonly sortOptions = Object.values(SortingOption) as SortingOption[] |
| 122 | + readonly sortState = toSignal(this.explorerSortService.sortState$, { requireSync: true }) |
| 123 | + readonly currentOption = computed(() => this.sortState()[0]) |
| 124 | + readonly isAscending = computed(() => this.sortState()[1]) |
| 125 | + |
| 126 | + @HostListener("document:click", ["$event"]) |
| 127 | + handleDocumentClick(event: MouseEvent) { |
| 128 | + if (!this.isOpen()) return |
| 129 | + if (!this.elementRef.nativeElement.contains(event.target as Node)) { |
| 130 | + this.isOpen.set(false) |
| 131 | + } |
| 132 | + } |
| 133 | + |
| 134 | + toggleOpen() { this.isOpen.update(v => !v) } |
| 135 | + |
| 136 | + setSortingOption(value: SortingOption) { |
| 137 | + this.explorerSortService.setSortingOption(value) |
| 138 | + this.isOpen.set(false) |
| 139 | + } |
| 140 | + |
| 141 | + toggleSortOrder() { |
| 142 | + this.explorerSortService.toggleSortingOrderAscending() |
| 143 | + this.isOpen.set(false) |
| 144 | + } |
| 145 | +} |
| 146 | +``` |
| 147 | + |
| 148 | +```html |
| 149 | +<!-- explorerSortControl.component.html (key bits) --> |
| 150 | +<div class="dropdown dropdown-start w-full px-2 mt-2" [class.dropdown-open]="isOpen()"> |
| 151 | + <button type="button" class="btn btn-ghost btn-sm w-full justify-between normal-case font-normal" |
| 152 | + [title]="'Sort: ' + currentOption()" |
| 153 | + (click)="toggleOpen()"> |
| 154 | + <!-- unchanged content --> |
| 155 | + </button> |
| 156 | + @if (isOpen()) { |
| 157 | + <ul class="dropdown-content menu bg-base-100 rounded-box z-20 w-full mx-2 p-2 shadow"> |
| 158 | + <!-- unchanged option list and order toggle --> |
| 159 | + </ul> |
| 160 | + } |
| 161 | +</div> |
| 162 | +``` |
| 163 | + |
| 164 | +## Performance Considerations |
| 165 | + |
| 166 | +- One additional signal in the component, read in two host bindings. Negligible. |
| 167 | +- `@if (isOpen())` removes the option `<ul>` from the DOM when closed — small CD/perf win versus today's always-rendered list (3 options + 1 separator). |
| 168 | + |
| 169 | +## Migration Notes |
| 170 | + |
| 171 | +- No persisted state; nothing to migrate. |
| 172 | +- Default state is **closed**, matching the visual default today. |
| 173 | +- Public API of the component is unchanged (selector `cc-explorer-sort-control`, no inputs/outputs). |
| 174 | + |
| 175 | +--- |
| 176 | + |
| 177 | +## Tasks |
| 178 | + |
| 179 | +### 1. Convert `ExplorerSortControlComponent` to a controlled dropdown |
| 180 | + |
| 181 | +- [x] Add `ElementRef` and `HostListener` imports; add `signal` import. Inject `ElementRef<HTMLElement>` as `elementRef`. |
| 182 | +- [x] Add `readonly isOpen = signal(false)` and a `toggleOpen()` method that flips it. |
| 183 | +- [x] Add `@HostListener("document:click", ["$event"]) handleDocumentClick(event)` that sets `isOpen` to `false` when the click target is outside `elementRef.nativeElement`. |
| 184 | +- [x] Update `setSortingOption(value)` and `toggleSortOrder()` to call `this.isOpen.set(false)` after dispatching to the service. |
| 185 | + |
| 186 | +### 2. Wire visibility into the template |
| 187 | + |
| 188 | +- [x] Add `[class.dropdown-open]="isOpen()"` to the outer `<div class="dropdown ...">`. |
| 189 | +- [x] Add `(click)="toggleOpen()"` to the trigger `<button>`. |
| 190 | +- [x] Wrap the `<ul class="dropdown-content ...">` in `@if (isOpen()) { ... }`. |
| 191 | + |
| 192 | +### 3. Cover the new behaviour with specs |
| 193 | + |
| 194 | +- [x] In `explorerSortControl.component.spec.ts`, add a test that the dropdown content is not rendered initially (`screen.queryByText(SortingOption.AREA_SIZE)` is null). |
| 195 | +- [x] Update the existing "should render every sorting option" test (currently asserts every option is in the DOM at render time) to first click the trigger to open the menu, then assert the options are visible. Required because once `@if (isOpen())` is added, the option `<ul>` is hidden by default and `getByText(SortingOption.NUMBER_OF_FILES)` / `getByText(SortingOption.AREA_SIZE)` would throw. |
| 196 | +- [x] Add a test that clicking the trigger button reveals the option list (`SortingOption.AREA_SIZE` becomes visible). |
| 197 | +- [x] Update the existing "dispatches setSortingOption" test to first click the trigger to open the menu, then click the option, then assert the option list is no longer in the DOM. |
| 198 | +- [x] Update the existing "dispatches toggleSortingOrderAscending" test the same way: open, click order toggle, assert closed. |
| 199 | +- [x] Add a test that clicking on `document.body` (outside the component) closes an open dropdown. |
| 200 | + |
| 201 | +### 4. Verify |
| 202 | + |
| 203 | +- [x] Run `npm test` from `visualization/`; new and existing specs pass. |
| 204 | +- [x] Run `npm run build` from `visualization/`; clean build. |
| 205 | +- [x] Run `npm run format:check` from the repo root. |
| 206 | +- [x] `npm run dev` and exercise the dropdown manually (see Manual Verification). |
| 207 | +- [x] Add a "Fixed 🐞" entry to `visualization/CHANGELOG.md` under `[unreleased]`. |
| 208 | + |
| 209 | +## Steps |
| 210 | + |
| 211 | +- [x] Complete Task 1: Convert `ExplorerSortControlComponent` to a controlled dropdown |
| 212 | +- [x] Complete Task 2: Wire visibility into the template |
| 213 | +- [x] Complete Task 3: Cover the new behaviour with specs |
| 214 | +- [x] Complete Task 4: Verify |
| 215 | + |
| 216 | +## Automated Verification |
| 217 | + |
| 218 | +- [x] New "trigger click opens menu" spec passes. |
| 219 | +- [x] Updated "setSortingOption closes menu" spec passes. |
| 220 | +- [x] Updated "toggleSortingOrderAscending closes menu" spec passes. |
| 221 | +- [x] New "outside click closes menu" spec passes. |
| 222 | +- [x] `npm test` passes. |
| 223 | +- [x] `npm run build` succeeds. |
| 224 | +- [x] `npm run format:check` passes. |
| 225 | + |
| 226 | +### Constraint gates (run from `visualization/`) |
| 227 | + |
| 228 | +- [x] OnPush preserved: `grep -c "ChangeDetectionStrategy.OnPush" app/codeCharta/features/sidebarExplorer/components/explorerSortControl/explorerSortControl.component.ts` returns `1`. |
| 229 | +- [x] No `setTimeout` / `NgZone` / manual CD in the changed file: `! grep -nE "setTimeout|NgZone|ChangeDetectorRef" app/codeCharta/features/sidebarExplorer/components/explorerSortControl/explorerSortControl.component.ts`. |
| 230 | +- [x] No new files outside the feature folder: `git diff --name-only main -- ':!visualization/app/codeCharta/features/sidebarExplorer/components/explorerSortControl/' ':!visualization/CHANGELOG.md' | wc -l` returns `0`. |
| 231 | + |
| 232 | +## Manual Verification (`npm run dev`) |
| 233 | + |
| 234 | +- [x] On load, the sort control shows `Sort: Name ↓` (or persisted state) with the dropdown closed. |
| 235 | +- [x] Clicking the trigger opens the menu listing `Name`, `Number of files`, `Area Size`, and a `Sort ascending / descending` row. |
| 236 | +- [x] Clicking any sort-key option (e.g. `Area Size`) updates the trigger label, re-sorts the file tree, and closes the menu. |
| 237 | +- [x] Clicking the order toggle row flips the arrow icon, re-sorts the tree, and closes the menu. |
| 238 | +- [x] Clicking the trigger again while the menu is open closes the menu without dispatching anything. |
| 239 | +- [x] Clicking anywhere outside the sort control while the menu is open closes the menu. |
| 240 | +- [x] No console warnings related to change detection or destroyed signals. |
| 241 | + |
| 242 | +## Notes |
| 243 | + |
| 244 | +- The fix mirrors `MapSelectorComponent` / `DeltaSelectorComponent` exactly so future readers see one consistent pattern in the project for controlled daisyUI dropdowns. |
| 245 | +- `@if (isOpen())` (rather than just `[class.dropdown-open]`) was chosen for parity with the two reference components and to avoid keeping the list mounted while hidden — small CD savings, no behavioural difference. |
| 246 | +- Default state is **closed**, matching today's visual default. |
| 247 | + |
| 248 | +## References |
| 249 | + |
| 250 | +- Sort control source: `visualization/app/codeCharta/features/sidebarExplorer/components/explorerSortControl/explorerSortControl.component.ts:1-27` |
| 251 | +- Sort control template: `visualization/app/codeCharta/features/sidebarExplorer/components/explorerSortControl/explorerSortControl.component.html:1-33` |
| 252 | +- Reference pattern (single-state): `visualization/app/codeCharta/features/navBar/components/mapSelector/mapSelector.component.ts:25,54-72,132` |
| 253 | +- Reference pattern (multi-state): `visualization/app/codeCharta/features/navBar/components/deltaSelector/deltaSelector.component.ts:23,49-71` |
| 254 | +- Sidebar redesign that introduced this control: `plans/2026-05-04-sidebar-explorer.md` |
0 commit comments