feat(platform): redesign filter system with improved UX#503
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the filter UI infrastructure in the data-table and filter components. The changes consolidate filter rendering through a new unified FilterSection-based approach, replacing separate RadioGroup and Checkbox rendering paths. The DataTableFilters component now uses FilterSection for both single-select (radio) and multi-select (checkbox) filters with updated styling and selection indicators. FilterSection receives new props (selectedCount, hasSelection) and updated header UI with chevron and selection badge display. Filter-related stories are updated to reflect the new structure and prop usage. Additionally, explicit multiSelect: false configurations are removed from filter definitions across multiple table components (customers, products, audit-logs, vendors, websites, and automations executions), changing their default selection behavior. Comprehensive test coverage is added for both DataTableFilters and FilterSection components. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/components/ui/data-table/data-table-filters.test.tsx`:
- Around line 122-123: The test uses a non-null assertion on the result of
screen.getByText('Active').closest('label') (variable activeLabel) which
triggers the typescript-eslint(no-non-null-assertion) rule; change this to
safely handle a possibly null element by using optional chaining or an explicit
guard: obtain the label via screen.getByText('Active')?.closest('label') or
assign without `!` and add a runtime check (e.g., if (!activeLabel) fail the
test or throw a clear error) before calling user.click(activeLabel) so the click
only runs when activeLabel is non-null.
- Around line 167-169: The test uses non-null assertions and an unnecessary type
assertion when locating the filter container: replace the
`checkboxes[0]!.closest('[class*="flex flex-col"]')!` pattern by safely querying
the DOM and asserting existence via testing-library helpers; for example, use a
direct query for the radiogroup or its parent (e.g., use
screen.getByRole('radiogroup') or screen.getByTestId / getByRole on a known
container) and pass that element into checkAccessibility, or guard the result
with expect(...).toBeInTheDocument() before calling checkAccessibility; update
the references to the `checkboxes`, `filterContent`, `screen.getAllByRole`, and
`checkAccessibility` calls accordingly to remove all non-null and unnecessary
type assertions.
In `@services/platform/app/components/ui/data-table/data-table-filters.tsx`:
- Around line 257-263: The radiogroup div with role="radiogroup" in the
DataTableFilters component lacks an accessible name; update that div to include
an aria-label or aria-labelledby (e.g., use the filter's label or id such as
filter.label or a generated heading id) so screen readers can identify the radio
group. Locate the div that uses role="radiogroup" and add aria-label={...} or
aria-labelledby="..." (ensuring the referenced label element exists and has a
matching id) while keeping the existing className/cn and filter.grid logic
intact.
In `@services/platform/app/components/ui/filters/filter-section.tsx`:
- Around line 33-37: The badge text currently uses a hardcoded English string;
update the FilterSection component to use the project's i18n key instead of the
literal "selected": replace the inline string that renders "{selectedCount}
selected" (in services/platform/app/components/ui/filters/filter-section.tsx,
where selectedCount is used) with the translated label (e.g., call the repo's
translation helper to render labels.selectedCount with the count interpolated).
Also add the labels.selectedCount = "{{count}} selected" entry to the common
translation files so interpolation works across locales.
Greptile SummaryRedesigns the filter system to default to single-select (radio) behavior instead of multi-select, replacing the Radix UI
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| services/platform/app/components/ui/data-table/data-table-filters.tsx | Core filter redesign: replaced RadioGroup with custom radio buttons, flipped multiSelect default to false. Missing aria-label on radiogroup and no arrow-key keyboard navigation. |
| services/platform/app/components/ui/filters/filter-section.tsx | Redesigned section header with chevron direction, uppercase title, selectedCount badge and hasSelection dot. Contains a hardcoded English string "selected" that bypasses i18n. |
| services/platform/app/components/ui/filters/filter-button.tsx | Added aria-label for accessibility using i18n translation. Clean, minimal change. |
| services/platform/app/components/ui/data-table/data-table-filters.test.tsx | New test file covering single-select, multi-select, grid layout, and accessibility audits. Good coverage of the new behavior. |
| services/platform/app/components/ui/filters/filter-section.test.tsx | New test file for FilterSection component with rendering, interaction, and accessibility tests. Thorough coverage. |
| services/platform/app/features/customers/components/customers-table.tsx | Removed multiSelect: false but left stray blank lines at lines 101 and 114. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DataTableFilters] --> B{filter.multiSelect?}
B -->|true| C[Checkbox UI]
B -->|false / undefined| D[Custom Radio UI]
C --> E[label + Checkbox component]
E --> F[handleCheckboxChange]
F --> G[filter.onChange with array add/remove]
D --> H[button role=radio]
H --> I[onClick toggle]
I --> J[filter.onChange with single value or empty]
A --> K[FilterSection]
K --> L{multiSelect?}
L -->|true| M[selectedCount badge]
L -->|false| N[hasSelection dot]
K --> O[ChevronDown with aria-expanded]
Last reviewed commit: 651b4df
…an up blank lines
Use t('labels.nSelected') instead of hardcoded string in FilterSection.
Mock useT in filter-section.test.tsx with interpolation support
so that t('labels.nSelected', { count: N }) returns 'N selected'
instead of the raw template string.
Summary
multiSelectflipped tofalse: Filters now default to single-select (radio) behavior, reducing friction for the most common use caseRadioGroupwith custom inline radio buttons featuring a filled circle indicator and row highlightingselectedCountbadge andhasSelectiondot indicator replace the oldactivebooleanaria-labelto filter button,aria-expandedto section togglesmultiSelect: falsedeclarations across 6 table components now that it's the defaultTest plan
multiSelect: trueSummary by CodeRabbit
New Features
Improvements