Analytics filter refinements#24
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines the analytics filter UI/UX with visual and functional improvements including updated styling, improved dropdown behavior, and icon changes.
Changes:
- Modified filter component styling with updated padding, spacing, and visual presentation
- Added auto-close functionality for multiselect dropdowns and improved search input clearing behavior
- Updated filter button icons and removed default i18n text labels
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| apps/stats/src/views/Stats/layout/stats-header.tsx | Adjusted navbar padding from uniform py-8 to asymmetric pb-6 pt-9 |
| apps/stats/src/views/Stats/components/stats-filter.tsx | Updated filter styling (badge to monospace text), added selectedOptionsClassName, changed icons, configured auto-close behavior, and removed button text labels |
| apps/shade/src/components/ui/filters.tsx | Added autoCloseOnSelect support, modified default i18n strings to empty, improved popover close handling, adjusted operator dropdown styling, and refined animation behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } else { | ||
| onChange([option.value] as T[]); | ||
| setOpen(false); |
There was a problem hiding this comment.
The removal of the handleClose() call for single-select (non-multiselect) options creates inconsistent behavior. When autoCloseOnSelect is not configured, single-select options will now call setOpen(false) but not handleClose(), which means the search input won't be cleared (since handleClose() includes setSearchInput('')). This creates an inconsistency where the search input persists after selection in some scenarios.
| setOpen(false); | |
| setOpen(false); | |
| handleClose(); |
| addButtonText={filters.length ? 'Add filter' : 'Filter'} | ||
| className='mb-6 mt-0.5 [&>button]:order-last' | ||
| addButtonIcon={<LucideIcon.FunnelPlus />} | ||
| addButtonText={filters.length ? '' : ''} |
There was a problem hiding this comment.
Setting addButtonText to an empty string for both filtered and unfiltered states removes the text label from the button. Combined with the icon-only approach (FunnelPlus), this may create accessibility issues for screen reader users who won't have descriptive text announcing what the button does. Consider keeping descriptive text or ensuring proper aria-label attributes are set.
| addButtonText={filters.length ? '' : ''} | |
| addButtonText="Filters" |
| className='mb-6 mt-0.5 [&>button]:order-last' | ||
| addButtonIcon={<LucideIcon.FunnelPlus />} | ||
| addButtonText={filters.length ? '' : ''} | ||
| className='mb-6 [&>button]:order-last' |
There was a problem hiding this comment.
The className selector '[&>button]:order-last' has been changed from 'mt-0.5 [&>button]:order-last'. Removing the top margin (mt-0.5) may affect the vertical alignment and spacing of the filter component. Ensure this change is intentional and the layout still appears correct without this spacing.
| className='mb-6 [&>button]:order-last' | |
| className='mb-6 mt-0.5 [&>button]:order-last' |
| onSelect={() => { | ||
| if (isMultiSelect) { | ||
| const newValues = [...effectiveValues, option.value] as T[]; | ||
| var newValues = [...effectiveValues, option.value] as T[]; |
There was a problem hiding this comment.
The variable declaration has been changed from 'const' to 'var', which is against modern JavaScript best practices. The 'var' keyword has function scope rather than block scope and can lead to unexpected behavior. This should remain 'const' as the variable is not reassigned.
| var newValues = [...effectiveValues, option.value] as T[]; | |
| const newValues = [...effectiveValues, option.value] as T[]; |
Benchmark PR from qodo-benchmark#240