BA-2169 BaseApp Activity Log - Filter by Date #200
Conversation
mathieubouhelier
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis PR introduces a date filtering feature for the activity log module. New components and type definitions have been added to support date selection via a chip that triggers either a mobile drawer or desktop menu. The activity log query and state management have been updated to incorporate the new filter parameters. Additionally, supporting changes include updates to constants, shared types, and documentation reflecting the new version. Changes
Possibly related PRs
Suggested reviewers
Poem
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
packages/components/modules/activity-log/DateFilterChip/types.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/activity-log/DateFilterComponent/types.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/activity-log/common/types.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (8)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
packages/components/modules/activity-log/DateFilterComponent/types.ts (1)
3-9: Enhance IFetchParameters interface.
- Add validation constraints using string literals for better type safety:
export interface IFetchParameters { createdFrom: string | null; createdTo: string | null; userName: string; count: number; cursor: string | null; /** Add pagination-related fields */ page?: number; /** Add sorting fields */ sortBy?: 'createdAt' | 'userName'; sortOrder?: 'asc' | 'desc'; }
- Consider adding
@minand@maxdecorators for thecountfield if you're using class-validator:import { Min, Max } from 'class-validator'; export interface IFetchParameters { // ...other fields @Min(1) @Max(100) count: number; }🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
packages/components/modules/__shared__/SearchNotFoundState/index.tsx (2)
Line range hint
1-22: Add prop-types validation and fix lint errors.
- Add prop-types validation:
+import PropTypes from 'prop-types'; import { SearchingImage } from '@baseapp-frontend/design-system'; import { Box, Typography } from '@mui/material'; import { SearchNotFoundStateProps } from '../types'; const SearchNotFoundState = ({ message = 'Check your spelling or try another search.', }: SearchNotFoundStateProps) => ( // ... component JSX ); +SearchNotFoundState.propTypes = { + message: PropTypes.string, +}; + +SearchNotFoundState.defaultProps = { + message: 'Check your spelling or try another search.', +}; export default SearchNotFoundState;
- Consider extracting the hardcoded strings to constants:
const DEFAULT_MESSAGE = 'Check your spelling or try another search.'; const NO_RESULTS_TITLE = 'No results found'; const SearchNotFoundState = ({ message = DEFAULT_MESSAGE, }: SearchNotFoundStateProps) => ( // ... component JSX with NO_RESULTS_TITLE );🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
10-20: Improve accessibility and i18n support.Consider these improvements:
- Add
aria-labelfor better screen reader support- Extract strings for i18n support
- Add test ID for e2e testing
<Box display="grid" justifyItems="center" gridAutoRows="min-content" gap={1.5} padding={4}> - <SearchingImage sx={{ color: 'grey.500' }} /> + <SearchingImage + sx={{ color: 'grey.500' }} + aria-label="No results illustration" + /> <Box display="grid" justifyItems="center" gridAutoRows="min-content" gap={0.5}> - <Typography variant="subtitle2" color="text.primary"> + <Typography + variant="subtitle2" + color="text.primary" + data-testid="no-results-title" + > {t('common.noResultsFound')} </Typography> - <Typography variant="caption" color="text.secondary"> + <Typography + variant="caption" + color="text.secondary" + data-testid="no-results-message" + > {t('common.searchMessage', { defaultValue: message })} </Typography> </Box> </Box>🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
packages/components/modules/__shared__/MobileDrawer/index.tsx (3)
7-15: Add accessibility attributes to the Puller component.The Puller component needs accessibility attributes to improve screen reader support.
const Puller = styled('div')(({ theme }) => ({ width: 64, height: 6, backgroundColor: theme.palette.grey[500], borderRadius: 3, position: 'absolute', top: 8, left: 'calc(50% - 32px)', + role: 'presentation', + 'aria-hidden': true, }))🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
20-20: Remove default title value.The default title "Drawer Title" is generic and might be accidentally left in production. Consider making the title prop required.
- title = 'Drawer Title', + title: string,🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
34-48: Add semantic HTML role and ARIA label to the content Box.Enhance accessibility by adding appropriate ARIA attributes to the content container.
<Box sx={{ position: 'relative', width: '100%', padding: 2, boxSizing: 'border-box', }} + role="dialog" + aria-label={title} >🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/DateFilterChip/index.tsx (2)
31-42: Add memoization to prevent unnecessary re-renders.The
labelRenderfunction is recreated on every render. Consider usinguseMemofor better performance.- const labelRender = () => { + const label = useMemo(() => { if (createdFrom && createdTo) { return `${dayjs(createdFrom).format('DD MMM YYYY')} - ${dayjs(createdTo).format('DD MMM YYYY')}` } if (createdFrom) { return `From ${dayjs(createdFrom).format('DD MMM YYYY')}` } if (createdTo) { return `Until ${dayjs(createdTo).format('DD MMM YYYY')}` } return 'Period' - } + }, [createdFrom, createdTo])🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
54-64: Enhance accessibility of the Chip component.Add ARIA attributes to improve screen reader support.
<Chip + role="button" + aria-haspopup="true" + aria-expanded={isMobile ? drawerOpen : Boolean(anchorEl)} label={ <Box sx={{ display: 'flex', alignItems: 'center', gap: 1 }}> - <span>{labelRender()}</span> + <span>{label}</span> <KeyboardArrowDownIcon color="action" /> </Box> } onClick={handleOnClickOnChip} variant={hasDateSelected ? 'filled' : 'soft'} color="default" />🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/DateFilterComponent/index.tsx (3)
38-50: Add loading state during refetch operation.The apply button should show a loading state while the refetch operation is in progress.
+ const [isLoading, setIsLoading] = useState(false) + const handleApply = async () => { if (tempCreatedTo && tempCreatedFrom && tempCreatedTo.isBefore(tempCreatedFrom)) { setError('minDate') return } setError(null) + setIsLoading(true) executeRefetch({ createdFrom: tempCreatedFrom?.format('YYYY-MM-DD') || null, createdTo: tempCreatedTo?.format('YYYY-MM-DD') || null, - }) + }).finally(() => { + setIsLoading(false) + }) onApply?.() }🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
67-91: Optimize component structure and reduce DOM nesting.Multiple nested Box components can be simplified for better performance.
- <Box display="flex" gap={2} flexDirection="column"> - <Box display="flex" gap={2} flexDirection="column"> + <Box + display="flex" + gap={2} + flexDirection="column" + sx={{ '& .MuiTextField-root': { width: '100%' } }} + > <LocalizationProvider dateAdapter={AdapterDayjs}> <DatePicker label="Start date" value={tempCreatedFrom} onChange={(newValue) => setTempCreatedFrom(newValue)} disableFuture /> <DatePicker label="End date" defaultValue={null} value={tempCreatedTo} onChange={(newValue) => setTempCreatedTo(newValue)} onError={(newError) => setError(newError)} disableFuture slotProps={{ textField: { helperText: errorMessage, }, }} /> </LocalizationProvider> - </Box>🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
92-98: Update button states and accessibility.Add loading state to the Filter button and improve button accessibility.
- <Button onClick={handleApply} variant="contained" color="inherit"> + <Button + onClick={handleApply} + variant="contained" + color="inherit" + disabled={isLoading} + aria-busy={isLoading} + > - Filter + {isLoading ? 'Filtering...' : 'Filter'} </Button> <Button onClick={handleClear} variant="outlined" color="inherit"> Clear Filter </Button>🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (2)
24-30: Consider making the page size configurable.The
countparameter is hardcoded to 10. Consider making it configurable through props to improve component reusability.const ActivityLogComponent: FC<ActivityLogComponentProps> = ({ queryRef, LogGroups = DefaultLogGroups, LogGroupsProps, + pageSize = 10, }) => { const [fetchParameters, setFetchParameters] = useState<IFetchParameters>({ createdFrom: null, createdTo: null, userName: '', - count: 10, + count: pageSize, cursor: null, })🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
84-88: Simplify date parameter checks.The null checks for date parameters could be simplified using the nullish coalescing operator.
- {!isPending && - (fetchParameters.createdFrom != null || fetchParameters.createdTo != null) && - emptyLogsList && ( + {!isPending && (fetchParameters.createdFrom ?? fetchParameters.createdTo) && emptyLogsList && ( <SearchNotFoundState message="No results found for the selected date range." /> )}🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/components/modules/__shared__/MobileDrawer/index.tsx(1 hunks)packages/components/modules/__shared__/SearchNotFoundState/index.tsx(1 hunks)packages/components/modules/__shared__/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(3 hunks)packages/components/modules/activity-log/DateFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/DateFilterComponent/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterComponent/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Main Workflow
packages/components/modules/activity-log/DateFilterChip/types.ts
[error] Lint command failed with exit code 2
packages/components/modules/__shared__/types.ts
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/DateFilterComponent/types.ts
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/DateFilterChip/index.tsx
[error] Lint command failed with exit code 2
packages/components/modules/__shared__/MobileDrawer/index.tsx
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/DateFilterComponent/index.tsx
[error] Lint command failed with exit code 2
packages/components/modules/__shared__/SearchNotFoundState/index.tsx
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
[error] Lint command failed with exit code 2
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (3)
12-13: LGTM! New imports align with date filtering feature.The imports are correctly structured and follow the existing pattern.
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
48-48: LGTM! Search functionality properly integrated with new fetch parameters.The search handlers correctly use the new
executeRefetchfunction while maintaining the existing functionality.Also applies to: 55-55
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
75-76: Verify gap value with design system.The gap value of 2 between filter chips should be verified against the design system for consistency.
✅ Verification successful
Gap value is consistent with design system patterns
The gap={2} spacing is verified to be consistent with other similar UI patterns across the codebase, particularly in flex container layouts for filter components and action items.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other gap values used in similar contexts rg -A 1 'gap=\{[2]\}' --type tsxLength of output: 62
Script:
#!/bin/bash # Search for gap usage in Box components with various spacing patterns rg -t ts -t tsx 'Box.*gap.*=.*\{[0-9]+\}' # Search for potential design system or theme files fd -e ts -e tsx -e js -e jsx '(theme|spacing|constants)'Length of output: 2710
Script:
#!/bin/bash # Search for gap usage with correct file extensions rg 'Box.*gap.*=.*\{[0-9]+\}' --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts # Check design system theme file cat packages/design-system/__mocks__/expo-constants.ts cat packages/components/__mocks__/theme.ts # Search for spacing or gap related constants rg 'spacing|gap' packages/design-system/Length of output: 9638
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] Lint command failed with exit code 2
packages/components/modules/activity-log/DateFilterChip/types.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
Show resolved
Hide resolved
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
3-8: Enhance the changelog entry to better document the feature scope.The current entry "add filter by date" is too brief. Consider expanding it to capture the key aspects of this feature implementation.
Apply this diff to improve the documentation:
## 0.0.58 ### Patch Changes -add filter by date +- Added date range filtering to Activity Log: + - Implemented date filter component with start and end date selection + - Added validation to prevent selecting future dates + - Enhanced UI with date range indicators and clear filter option
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
mathieubouhelier
force-pushed
the
BA-2169-fe-package-base-app-activity-log-filter-by-date
branch
from
060eb2b to
9edec05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (2)
52-57: Optimize search performance with debouncing.Consider adding debounce to the search handler to prevent excessive API calls while typing.
+import { useMemo } from 'react' +import debounce from 'lodash/debounce' const handleSearchChange = (e: ChangeEvent<HTMLInputElement>) => { const value = e.target.value || '' startTransition(() => { executeRefetch({ userName: value }) }) } +const debouncedHandleSearchChange = useMemo( + () => debounce(handleSearchChange, 300), + [] +)
75-88: Enhance accessibility for filter components.Consider adding ARIA labels and roles to improve screen reader support for the filter components.
-<Box display="flex" mt={2} mb={2} flexDirection="row" gap={2}> +<Box + display="flex" + mt={2} + mb={2} + flexDirection="row" + gap={2} + role="group" + aria-label="Activity log filters" +>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/MobileDrawer/index.tsx(1 hunks)packages/components/modules/__shared__/SearchNotFoundState/index.tsx(1 hunks)packages/components/modules/__shared__/types.ts(1 hunks)packages/components/modules/activity-log/ActivityLogComponent/index.tsx(3 hunks)packages/components/modules/activity-log/DateFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/DateFilterComponent/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterComponent/types.ts(1 hunks)packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/package.json(1 hunks)packages/utils/constants/date.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/components/package.json
- packages/utils/constants/date.ts
- packages/components/modules/activity-log/DateFilterChip/index.tsx
- packages/components/modules/shared/types.ts
- packages/components/modules/activity-log/DateFilterChip/types.ts
- packages/components/modules/activity-log/DateFilterComponent/types.ts
- packages/components/modules/shared/SearchNotFoundState/index.tsx
- packages/components/CHANGELOG.md
- packages/components/modules/shared/MobileDrawer/index.tsx
- packages/components/modules/activity-log/DateFilterComponent/index.tsx
- packages/components/modules/activity-log/graphql/queries/ActivityLogsFragment.ts
🔇 Additional comments (2)
packages/components/modules/activity-log/ActivityLogComponent/index.tsx (2)
12-13: LGTM! Import changes align with feature requirements.The new imports support the date filtering feature requirements.
37-43: Add error handling for refetch operations.The
executeRefetchfunction should handle potential failures during the refetch operation to provide better error feedback to users.
packages/components/modules/activity-log/ActivityLogComponent/index.tsx
Outdated
Show resolved
Hide resolved
mathieubouhelier
force-pushed
the
BA-2169-fe-package-base-app-activity-log-filter-by-date
branch
from
9edec05 to
3f98849
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/modules/activity-log/common/graphql/queries/ActivityLogsFragment.ts (2)
26-29: Consider pagination performance with date filters.While the
@connectiondirective is correctly configured, be aware that including date filters in the connection key will create separate pagination states for each unique date range. This means the cache won't be reused between different date selections, potentially leading to more network requests.Consider implementing cursor-based pagination with date boundaries to optimize performance for large datasets.
78-81: Consider date-aware log grouping.The current grouping logic uses a fixed 15-minute window. With the new date filtering capabilities, consider enhancing this to be date-aware, potentially grouping by day first and then by the time window. This would provide a more intuitive view when users filter by date ranges.
if ( !lastGroup || - timestamp - new Date(lastGroup.lastActivityTimestamp).getTime() > 15 * 60 * 1000 + new Date(timestamp).toDateString() !== new Date(lastGroup.lastActivityTimestamp).toDateString() || + timestamp - new Date(lastGroup.lastActivityTimestamp).getTime() > 15 * 60 * 1000 ) {packages/components/modules/activity-log/web/ActivityLogComponent/index.tsx (1)
83-87: Consider combining the SearchNotFoundState conditions.The separate conditions for search value and date filters could be combined to reduce code duplication and improve maintainability.
- {!isPending && searchValue && emptyLogsList && <SearchNotFoundState />} - {!isPending && - (fetchParameters.createdFrom != null || fetchParameters.createdTo != null) && - emptyLogsList && ( - <SearchNotFoundState message="No results found for the selected date range." /> - )} + {!isPending && emptyLogsList && ( + <SearchNotFoundState + message={ + fetchParameters.createdFrom != null || fetchParameters.createdTo != null + ? "No results found for the selected date range." + : searchValue + ? "Check your spelling or try another search." + : "No activities found." + } + /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/MobileDrawer/index.tsx(1 hunks)packages/components/modules/__shared__/common/types.ts(1 hunks)packages/components/modules/__shared__/web/SearchNotFoundState/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/DateFilterComponent/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterComponent/types.ts(1 hunks)packages/components/modules/activity-log/common/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/web/ActivityLogComponent/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/CHANGELOG.md
- packages/components/modules/activity-log/DateFilterComponent/types.ts
- packages/components/modules/shared/MobileDrawer/index.tsx
- packages/components/modules/activity-log/DateFilterComponent/index.tsx
- packages/components/modules/activity-log/DateFilterChip/index.tsx
- packages/components/modules/activity-log/DateFilterChip/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (7)
packages/components/modules/activity-log/common/graphql/queries/ActivityLogsFragment.ts (2)
16-17: LGTM! Well-defined argument definitions for date filtering.The new
createdFromandcreatedToarguments are appropriately typed and defaulted, providing the necessary flexibility for optional date filtering.
19-25: LGTM! Query parameters properly configured.The
activityLogsquery parameters are correctly structured to include the new date filters while maintaining existing pagination and username filtering capabilities.packages/components/modules/__shared__/common/types.ts (1)
1-16: LGTM! Well-structured type definitions.The type definitions are clear, well-organized, and follow TypeScript best practices. The optional properties are appropriately marked, and the ReactNode type is correctly imported for the children prop.
packages/components/modules/__shared__/web/SearchNotFoundState/index.tsx (1)
5-9: LGTM! Good component enhancement.The changes improve component reusability by making the message customizable while maintaining a sensible default. The type definitions are properly imported and used.
Also applies to: 17-17
packages/components/modules/activity-log/web/ActivityLogComponent/index.tsx (3)
23-29: LGTM! Well-structured state initialization.The fetch parameters state is well-organized with proper typing and null initialization for optional fields.
36-42: LGTM! Good centralization of refetch logic.The executeRefetch function effectively centralizes the refetch logic and properly handles state updates with transitions for smooth UI updates.
74-81:❓ Verification inconclusive
Verify accessibility of the filter chips.
Please ensure that the filter chips are keyboard accessible and have proper ARIA labels for screen readers.
🌐 Web query:
What are the accessibility requirements for MUI Chip components?💡 Result:
MUI Chip components require adherence to accessibility standards to ensure usability for all users, including those relying on assistive technologies. Based on WCAG guidelines and Material-UI's implementation practices:
ARIA Compliance
- Role & Attributes: Use
role="button"for interactive chips[2]. Selected chips requirearia-pressed="true", while unselected usearia-pressed="false"[2][9].- Labels: Provide accessible names via
aria-labelor associated<label>elements[3][9]. Avoid relying solely on visual cues like color.Keyboard Navigation
- Focus: Ensure chips are included in the tab sequence (
Tabkey)[3][9].- Actions:
Enter/Spacetriggers click events[2][4]Delete/Backspaceremoves deletable chips[2][4]- Navigation: Arrow keys should move focus between chips in groups[4]
Focus Management
- Visible focus indicators using CSS
:focusor:focus-visiblepseudoclasses[3]- Maintain logical tab order when dynamically adding/removing chips[7]
Color Contrast
- Meet WCAG AA minimums:
- 4.5:1 contrast for small text
- 3:1 for large text (18pt+)[5][8]
- Test outlined variants carefully - some MUI colors (info/warning) fail contrast checks in default themes[5]
Screen Reader Support
- Announce chip state changes (selected/deselected)[2]
- Handle dynamic content updates with ARIA live regions[7]
- Test with popular screen readers:
- NVDA + Firefox
- JAWS + Chrome/Edge[2]
Testing Requirements
- Automated checks with tools like axe-core[2]
- Manual verification using keyboard-only navigation[7]
- Validate color contrast ratios during theme customization[5][8]
For implementation, MUI Chips automatically handle many accessibility features when properly configured[9], but developers must:
- Add appropriate labels
- Ensure color contrast compliance
- Test keyboard interactions
- Verify screen reader announcements
Common pitfalls include insufficient contrast in outlined variants[5] and missing ARIA labels in dynamic chip lists[7]. Regular accessibility audits are recommended when customizing chip designs[8].
Citations:
- 1: https://blogs.purecode.ai/blogs/mui-chip
- 2: https://www.telerik.com/design-system/docs/components/chip/accessibility/
- 3: https://mui.com/base-ui/getting-started/accessibility/
- 4: https://v4.mui.com/components/data-grid/accessibility/
- 5: mui/material-ui#30084
- 6: https://djibe.github.io/material/docs/4.6/material/chips/
- 7: mui/material-ui#17708
- 8: https://www.accessibilitycloud.com/comparison-and-accessibility-audit-of-semantic-mui-and-carbon-design/
- 9: https://mui.com/material-ui/react-chip/
- 10: https://mui.com/material-ui/api/chip/
Ensure Filter Chips are Fully Accessible
Please verify that both the DateFilterChip and EventFilterChip components meet WCAG accessibility standards. In particular:
- Confirm that interactive chip elements have the appropriate ARIA attributes (e.g., use
role="button"and setaria-pressedfor toggle states where applicable, as well as providing descriptivearia-labelvalues).- Ensure that the chips are keyboard navigable (included in the Tab order with clear focus indicators) and that they respond to key events (e.g., Enter/Space for activation).
- Validate that any dynamic updates or state changes are communicated properly to screen readers.
These adjustments help guarantee that all users, including those relying on assistive technologies, have an optimal experience.
anicioalexandre
left a comment
anicioalexandre
left a comment
There was a problem hiding this comment.
maybe we could do it in a bug, but the log timeline looks a bit off, it should continue until we reach the next profile avatar, atm the data is getting on the way. @priscilladeroode can you create a story to double check the UI on the activity-log module?
mathieubouhelier
force-pushed
the
BA-2169-fe-package-base-app-activity-log-filter-by-date
branch
from
3f98849 to
5fd80ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/components/modules/activity-log/DateFilterComponent/index.tsx (2)
34-46: Reset error state when dates change.Currently, the error state is only reset when applying the filter. Consider resetting it when either date changes to provide immediate feedback.
You could modify the date change handlers:
- const handleApply = () => { + const handleApply = () => { if (tempCreatedTo && tempCreatedFrom && tempCreatedTo < tempCreatedFrom) { setError('minDate') return } setError(null) executeRefetch({ createdFrom: tempCreatedFrom ? tempCreatedFrom.toISODate() : null, createdTo: tempCreatedTo ? tempCreatedTo.toISODate() : null, }) onApply?.() } + const handleDateChange = (isStartDate: boolean, newValue: DateTime | null) => { + setError(null) + if (isStartDate) { + setTempCreatedFrom(newValue) + } else { + setTempCreatedTo(newValue) + } + }Then update the DatePicker onChange props:
- onChange={(newValue) => setTempCreatedFrom(newValue)} + onChange={(newValue) => handleDateChange(true, newValue)} - onChange={(newValue) => setTempCreatedTo(newValue)} + onChange={(newValue) => handleDateChange(false, newValue)}
64-113: Consider adding aria-labels for better accessibility.The date picker components could benefit from additional accessibility attributes to improve the experience for screen reader users.
<DatePicker label="Start date" onChange={(newValue) => setTempCreatedFrom(newValue)} disableFuture value={tempCreatedFrom} shouldDisableDate={disableStartDate} + aria-label="Filter activities from this start date" slotProps={{ day: (dayProps) => ({ sx: { ...(disableStartDate(dayProps.day) && { backgroundColor: 'error.lighter', }), }, }), }} /> <DatePicker label="End date" value={tempCreatedTo} onChange={(newValue) => setTempCreatedTo(newValue)} onError={(newError) => setError(newError)} disableFuture shouldDisableDate={disableEndDate} + aria-label="Filter activities until this end date" slotProps={{packages/components/modules/activity-log/web/ActivityLogComponent/index.tsx (1)
83-87: Improve readability of conditional rendering.The nested conditional rendering for the "no results" state could be more readable with a separate conditional variable.
- {!isPending && - (fetchParameters.createdFrom != null || fetchParameters.createdTo != null) && - emptyLogsList && ( - <SearchNotFoundState message="No results found for the selected date range." /> - )} + {!isPending && emptyLogsList && ( + <> + {searchValue && <SearchNotFoundState />} + {(fetchParameters.createdFrom != null || fetchParameters.createdTo != null) && ( + <SearchNotFoundState message="No results found for the selected date range." /> + )} + </> + )}Or alternatively:
+ const hasDateFilter = fetchParameters.createdFrom != null || fetchParameters.createdTo != null; + const shouldShowNoResultsForDateFilter = !isPending && hasDateFilter && emptyLogsList; + const shouldShowNoResultsForSearch = !isPending && searchValue && emptyLogsList; {!isPending && searchValue && emptyLogsList && <SearchNotFoundState />} - {!isPending && - (fetchParameters.createdFrom != null || fetchParameters.createdTo != null) && - emptyLogsList && ( - <SearchNotFoundState message="No results found for the selected date range." /> - )} + {shouldShowNoResultsForDateFilter && ( + <SearchNotFoundState message="No results found for the selected date range." /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (11)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/__shared__/common/types.ts(1 hunks)packages/components/modules/__shared__/web/SearchNotFoundState/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterChip/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterChip/types.ts(1 hunks)packages/components/modules/activity-log/DateFilterComponent/index.tsx(1 hunks)packages/components/modules/activity-log/DateFilterComponent/types.ts(1 hunks)packages/components/modules/activity-log/common/graphql/queries/ActivityLogsFragment.ts(1 hunks)packages/components/modules/activity-log/web/ActivityLogComponent/index.tsx(3 hunks)packages/components/package.json(1 hunks)packages/utils/constants/date.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/utils/constants/date.ts
- packages/components/modules/activity-log/DateFilterChip/types.ts
- packages/components/modules/activity-log/common/graphql/queries/ActivityLogsFragment.ts
- packages/components/modules/shared/web/SearchNotFoundState/index.tsx
- packages/components/modules/shared/common/types.ts
- packages/components/modules/activity-log/DateFilterComponent/types.ts
- packages/components/modules/activity-log/DateFilterChip/index.tsx
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[misspelling] ~7-~7: Did you mean “done” (= past participle of “do”)?
Context: ...op and swap on mobile. Filtering can be don by start and/or end date ## 1.0.11 ##...
(DON_DONE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (8)
packages/components/CHANGELOG.md (1)
3-7: Add more specific details to the changelog entry.The changelog entry could be more specific about the module being modified and include more details about the implementation.
Consider enhancing the description with more details like:
-add filter by date to the module activity log. Filter as a modal on desktop and swap on mobile. Filtering can be done by start and/or end date +add filter by date to the module activity log. Filter as a modal on desktop and swap on mobile. Filtering can be done by start and/or end date. Implemented using DateFilterChip component that integrates with the ActivityLogComponent.🧰 Tools
🪛 LanguageTool
[misspelling] ~7-~7: Did you mean “done” (= past participle of “do”)?
Context: ...op and swap on mobile. Filtering can be don by start and/or end date ## 1.0.11 ##...(DON_DONE)
packages/components/modules/activity-log/DateFilterComponent/index.tsx (3)
23-32: LGTM! Clear error handling for date validation.The error message handling is well-implemented with clear, user-friendly messages for different validation scenarios.
60-63: LGTM! Good date constraint implementation.The logic to disable dates based on the selected range is well-implemented, preventing users from selecting invalid date combinations.
48-58:❓ Verification inconclusive
Verify the clearing filter logic.
The handleClear function checks if createdFrom or createdTo exists before performing a refetch, but always resets the UI state. This could potentially lead to inconsistency between UI and data state.
🏁 Script executed:
#!/bin/bash # Search for usages of handleClear to understand how it's used in context rg -A 5 "handleClear" --type tsxLength of output: 60
Clarify and verify the clear filter behavior in DateFilterComponent.
The current implementation resets the temporary UI state unconditionally—clearing both
tempCreatedFromandtempCreatedTo—while only triggering a refetch if eithercreatedFromorcreatedToexists. This approach may be intentional; however, it warrants a closer manual review to ensure that clearing the local UI state without a corresponding data update (when no filter is active) does not lead to any unintended inconsistencies.
- File:
packages/components/modules/activity-log/DateFilterComponent/index.tsx(lines 48-58)- Key Check: Confirm that always resetting the temporary filters aligns with the overall filter update strategy, especially for cases when no data filter is applied.
- Next Step: Please verify if this design is intentional or consider adjusting the logic so that the UI state and data state remain consistent in all scenarios.
packages/components/modules/activity-log/web/ActivityLogComponent/index.tsx (4)
23-29: LGTM! Well-structured state management for filter parameters.The fetchParameters state object is well-designed, encompassing all necessary filtering parameters in a single state variable.
36-42: LGTM! Excellent refactoring of refetch logic.The executeRefetch function centralizes the parameter updating and refetch logic, making the code more maintainable and reducing duplication.
74-76: LGTM! Clean integration of the DateFilterChip component.The DateFilterChip component is well-integrated with appropriate props, creating a consistent UI with the existing EventFilterChip.
11-12:❓ Verification inconclusive
Verify the interaction between DateFilterChip and ActivityLogComponent.
The integration of DateFilterChip with ActivityLogComponent seems straightforward, but it's worth verifying that the filter parameters are properly passed and processed.
🏁 Script executed:
#!/bin/bash # Look for the DateFilterChip implementation to understand how it handles the parameters rg -A 10 "const DateFilterChip" --type tsxLength of output: 72
Below is an updated verification script designed to correctly search TSX files for the implementation of DateFilterChip, which will help us assess whether its handling of filter parameters matches the integration in ActivityLogComponent:
#!/bin/bash # Search for implementations (both const and function declaration forms) of DateFilterChip in TSX files. rg -A 10 -e "const DateFilterChip" -e "function DateFilterChip" -g "*.tsx"This revised command avoids the unrecognized file type issue seen earlier. Once you run this script and confirm the output, please verify that the filter parameters are indeed correctly processed by DateFilterChip.
Next steps:
- Confirm that DateFilterChip’s implementation shows proper handling of filter parameters.
- Ensure that ActivityLogComponent passes the expected parameters without any issues.
packages/components/modules/activity-log/DateFilterComponent/types.ts
Outdated
Show resolved
Hide resolved
|
nossila
deleted the
BA-2169-fe-package-base-app-activity-log-filter-by-date
branch
Description
As a user, on the Baseapp Activity Log,I would like to Filter activity entries by date, In order to quickly search for activities made in specific periods of time.
Acceptance Criteria
Context
The objective of this story is to implement a filter that reduces the amount of activity logs displayed in order to search for logs in a specific period of time.
Business Rules
Given I am a user searching for specific activites, when I use the date filters, Then the system shall display all activity logs that occurred from the start date through the end date, inclusive of both dates.
The system must provide a user-friendly interface for selecting a start date and an end date for filtering the activity logs.
Make sure the component complies with the current implemented Design System
The date selection tool must prevent the user from selecting an end date that is earlier than the start date.
The filtered activity log view must include a clear indicator of the date range currently being viewed per figma
Users must have the ability to clear the selected date range to return to viewing all activity logs without applying the date filter individually
The filter must apply after a date has been set, even if it's only 1 date that has been selected
The user should not be allowed to select future dates.
Design Link: https://www.figma.com/design/XRD6wSl1m8Kz6XUcAy5CLp/BaseApp---WEB?node-id=3738-46140&t=0SETItr5R9RznbuZ-4
DEMO:
https://www.loom.com/share/dec2d3ab87984a85acc52ccce6f20a81
Summary by CodeRabbit
New Features
Chores