Skip to content

ECHOES-1362 Implement FilterDropdown component#709

Merged
gregaubert merged 3 commits into
mainfrom
greg/filterdropdown
Jun 26, 2026
Merged

ECHOES-1362 Implement FilterDropdown component#709
gregaubert merged 3 commits into
mainfrom
greg/filterdropdown

Conversation

@gregaubert

@gregaubert gregaubert commented Jun 23, 2026

Copy link
Copy Markdown
Member

Summary by Gitar

  • New component:
    • Implemented FilterDropdown, a multi-column popover featuring multi-select, single-select, search, and async loading states.
  • Accessibility & UX:
    • Added custom useFilterDropdownRovingFocus hook for keyboard-driven navigation across panel rows.
    • Implemented comprehensive keyboard interaction support (e.g., arrow keys for panel switching and item navigation).
  • Design tokens & Styles:
    • Added filter.dropdown sizing tokens in base.json.
    • Created shared styleDropdownItemBase to unify styling for FilterDropdown, DropdownMenu, and related components.
  • Testing & Storybook:
    • Added extensive test suite in FilterDropdown-test.tsx including accessibility compliance.
    • Added interactive stories in FilterDropdown-stories.tsx covering various configurations.

This will update automatically on new commits.

@netlify

netlify Bot commented Jun 23, 2026

Copy link
Copy Markdown

Deploy Preview for echoes-react ready!

Name Link
🔨 Latest commit 512492f
🔍 Latest deploy log https://app.netlify.com/projects/echoes-react/deploys/6a3d476ced734900083d3c0c
😎 Deploy Preview https://deploy-preview-709--echoes-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

Comment thread src/components/filters/FilterDropdown.tsx Outdated
Comment thread src/components/filters/FilterDropdown.tsx Outdated
Comment thread src/components/filters/FilterDropdown.tsx Outdated
@gregaubert gregaubert force-pushed the greg/filterdropdown branch 4 times, most recently from a4eefca to fb1164a Compare June 24, 2026 15:24
@gregaubert gregaubert marked this pull request as ready for review June 24, 2026 15:24
Comment thread src/components/filters/FilterDropdown.tsx Outdated
Comment thread src/common/helpers/constants.ts Outdated
@gregaubert gregaubert force-pushed the greg/filterdropdown branch from fb1164a to 078e373 Compare June 24, 2026 15:33
Comment thread src/components/filters/FilterDropdownItemsList.tsx Outdated
Comment thread src/components/dropdown-menu/DropdownMenuStyles.tsx
Comment thread src/components/filters/FilterDropdownRightPanel.tsx
Comment thread src/components/filters/FilterDropdown.tsx
Comment thread stories/filters/FilterDropdown-stories.tsx
@gregaubert gregaubert force-pushed the greg/filterdropdown branch from 078e373 to eb536bd Compare June 25, 2026 09:53

@jeremy-davis-sonarsource jeremy-davis-sonarsource left a comment

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.

Awesome work! It looks great.

Comment thread src/components/radio-button-group/RadioButton.tsx
Comment thread src/components/filters/FilterDropdownItemsList.tsx Outdated
Comment thread stories/filters/FilterDropdown-stories.tsx Outdated
Comment thread src/components/filters/useFilterDropdownKeyboardThrottle.ts
@gregaubert gregaubert force-pushed the greg/filterdropdown branch from 282eace to e00c0d0 Compare June 25, 2026 15:20
@gregaubert gregaubert force-pushed the greg/filterdropdown branch from e00c0d0 to 512492f Compare June 25, 2026 15:21
@sonarqube-next

Copy link
Copy Markdown

@gregaubert gregaubert merged commit 369745d into main Jun 26, 2026
10 checks passed
@gregaubert gregaubert deleted the greg/filterdropdown branch June 26, 2026 07:45
@gitar-bot

gitar-bot Bot commented Jun 26, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 11 resolved / 11 findings

Implements the FilterDropdown component with multi-column support, keyboard navigation hooks, and design tokens. All previously identified issues regarding event handling, navigation, and state management have been resolved.

✅ 11 resolved
Bug: Built-in search filter not skipped when onSearch is provided

📄 src/components/filters/FilterDropdown.tsx:109-116 📄 src/components/filters/FilterDropdownTypes.ts:81-88
The onSearch prop is documented (FilterDropdownTypes.ts:81-88) as: "When provided, the component skips its built-in client-side filtering and delegates search entirely to the consumer." However, filteredItems (FilterDropdown.tsx:109-116) applies the local label-substring filter whenever activeCategory?.isSearchable is true, regardless of whether onSearch is defined.

Consequence: when a consumer supplies onSearch and returns server-filtered items that do not literally contain the query as a label substring (fuzzy/relevance matching, matching on non-label fields, accent-insensitive search, etc.), the component will incorrectly filter those results out a second time, hiding valid items. This breaks the documented delegation behavior.

Fix: only apply the built-in filter when onSearch is not provided for the active category.

Bug: onClose not fired when closing via Apply or Clear

📄 src/components/filters/FilterDropdown.tsx:185-194 📄 src/components/filters/FilterDropdown.tsx:280 📄 src/components/filters/FilterDropdownTypes.ts:156-159 📄 src/components/filters/FilterDropdown.tsx:134-143 📄 src/components/filters/FilterDropdown.tsx:179-180
handleApply (line 187) and handleClear (line 193) close the popover by calling setLocalIsOpen(false) directly, bypassing handleOpenChange. Because RadixPopover.Root is used in controlled mode (open={open} is always passed), Radix's internal useControllableState does NOT invoke onOpenChange when the controlled open prop changes programmatically — it only fires on user-driven interactions (Escape, outside click, trigger). As a result, onClose is only called on Escape/outside-click, not on Apply or Clear.

This contradicts the documented contract in FilterDropdownTypes.ts: onClose is described as "Called when the popover closes (whether by Apply, Clear, or Escape/outside click)." Consumers relying on onClose for cleanup/analytics will miss the Apply and Clear close paths. Additionally, in controlled mode (isOpen provided), setLocalIsOpen(false) has no effect on the rendered open value and there is no callback to tell the controller to close, so the popover stays open after Apply/Clear.

Fix: route both handlers through the close path so onClose fires consistently, e.g. call handleOpenChange(false) (or explicitly invoke onClose?.()) from handleApply/handleClear. There is also no test covering onClose.

Edge Case: Single-select current value derived from unfiltered items

📄 src/components/filters/FilterDropdownRightPanel.tsx:65-75 📄 src/components/filters/FilterDropdownRightPanel.tsx:124-134
In FilterDropdownRightPanel, activeCategoryCurrentValue is derived from the unfiltered items list, while the rendered FilterDropdownSingleSelectList receives filteredItems ?? []. For a single-select category that is also isSearchable (client-side), if the search query hides the currently-selected option, the RadioGroup value still points to an option that is not rendered. Radix will hold a selected value with no matching radio in the DOM, which can produce inconsistent selection/focus behavior. Consider deriving currentValue from the same filtered list that is rendered.

Edge Case: Arrow-key nav doesn't skip disabled checkbox items

📄 src/components/filters/FilterDropdownItemsList.tsx:81-95 📄 src/components/filters/FilterDropdownItemsList.tsx:103-116
In FilterDropdownMultiSelectList.handleKeyDown, ArrowDown/ArrowUp compute the next index with Math.min(activeItemIndex + 1, items.length - 1) without considering item.isDisabled. The multi-select row is a RadixCheckbox.Root rendered with disabled={isDisabled}, i.e. a disabled <button> that cannot receive focus. When navigation lands on a disabled item, itemFocus(nextIndex) is a no-op (the disabled button cannot be focused), yet activeItemIndex has been moved to that index and the roving tabIndex=0 is now assigned to an unfocusable element while the previously focused item keeps tabIndex=-1. This leaves focus in an inconsistent state and the disabled item becomes the only tab stop until another arrow press. The single-select path is unaffected because Radix RadioGroup natively skips disabled radios. Consider skipping disabled items when computing the next/previous index.

Quality: Controlled isOpen has no onOpenChange to drive close

📄 src/components/filters/FilterDropdown.tsx:80 📄 src/components/filters/FilterDropdown.tsx:84-98 📄 src/components/filters/FilterDropdownTypes.ts:109-113
FilterDropdown accepts a controlled isOpen prop (open = isOpen ?? localIsOpen) but only ever updates internal localIsOpen inside handleOpenChange. When a consumer fully controls isOpen, Escape / outside-click / Apply / Clear all call handleOpenChange(false) which updates localIsOpen (ignored) and fires onClose, but the popover cannot actually close unless the consumer flips isOpen themselves in response to onClose/onOpen. The isOpen TSDoc says only "Controls the open state externally" and doesn't document this requirement, making the controlled mode easy to misuse. Either document that controlled usage must update isOpen via onOpen/onClose, or expose an onOpenChange callback.

...and 6 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

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.

2 participants