[Feat]: Implement dynamic date range picker for timesheet filtering #3213
Conversation
WalkthroughThe changes in this pull request include the modification of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (1)
Line range hint
27-33: Consider implementing loading state for async operations.The date range filtering will likely involve API calls to fetch filtered timesheet data. Consider adding loading states to provide better user feedback during these operations.
<TimesheetFilterDate /> <Button onClick={() => null} + disabled={isLoading} variant='outline' className='bg-primary/5 dark:bg-primary-light h-10 w-[2.5rem] font-medium'> - Add Time + {isLoading ? 'Loading...' : 'Add Time'} </Button>apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx (1)
79-92: Enhance DatePickerInput component with better type safety and visual feedback.Consider adding proper type safety for the date format and improving the hover state feedback.
const DatePickerInput: React.FC<DatePickerInputProps> = ({ date, label }) => ( <> <Button variant="outline" className={cn( "w-[150px] justify-start text-left font-normal bg-transparent hover:bg-transparent text-black h-8 border border-transparent dark:border-transparent", - !date && "text-muted-foreground" + !date && "text-muted-foreground", + "hover:border-primary transition-colors duration-200" )} + title={date ? format(date, "PPP") : label} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx(2 hunks)apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx(1 hunks)apps/web/app/[locale]/timesheet/components/index.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (2)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (2)
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx:67-79
Timestamp: 2024-10-30T16:25:19.910Z
Learning: The date range selection functionality for the date range options in the `FilterTaskActionMenu` component will be implemented in a future PR.
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx:30-35
Timestamp: 2024-10-30T16:31:01.294Z
Learning: In the `TimesheetFilter` component, the `Add Time` button does not need to use the `AddManualTimeModal` component, as per the user's decision.
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx (1)
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx:67-79
Timestamp: 2024-10-30T16:25:19.910Z
Learning: The date range selection functionality for the date range options in the `FilterTaskActionMenu` component will be implemented in a future PR.
🔇 Additional comments (3)
apps/web/app/[locale]/timesheet/components/index.tsx (1)
7-7: LGTM! Verify the exported component file exists.
The new export statement follows the established pattern and aligns with the PR's objective of implementing date range filtering for timesheets.
Let's verify the existence of the TimesheetFilterDate component:
✅ Verification successful
Export statement is valid and component file exists ✅
The TimesheetFilterDate component file exists at the expected location: apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx, making this export statement valid and properly structured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the TimesheetFilterDate component file exists
# Expected: Find the component file in the timesheet components directory
fd --type f "TimesheetFilterDate.tsx" apps/web/app/
Length of output: 118
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (1)
1-2: LGTM! Clean import organization.
The addition of TimesheetFilterDate to the barrel import maintains consistency with the existing pattern.
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx (1)
1-18: LGTM! Clean imports and interface definition.
The imports are well-organized and the interface is appropriately defined for its purpose.
| @@ -25,6 +24,7 @@ export function TimesheetFilter() { | |||
| <SettingFilterIcon className="text-gray-700 dark:text-white w-3.5" strokeWidth="1.8" /> | |||
| <span className="text-gray-700 dark:text-white">Filter</span> | |||
| </button> | |||
| <TimesheetFilterDate /> | |||
There was a problem hiding this comment.
💡 Codebase verification
Date range selection is not connected to timesheet filtering
The TimesheetFilterDate component properly implements local state management for date range selection using useState, but it's not connected to any parent component or filtering logic:
- The component maintains its own state with
dateRangeand handlershandleFromChange/handleToChange - No props are passed to TimesheetFilterDate from TimesheetFilter
- No context or event handlers are found to propagate the selected date range
- The parent TimesheetFilter component doesn't handle or use the selected dates
🔗 Analysis chain
Verify state management for date range selection.
The TimesheetFilterDate component is added but there's no visible state management or event handling for the selected date range. This might lead to the date range selection not affecting the timesheet filtering.
Let's verify the implementation of TimesheetFilterDate:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if TimesheetFilterDate component exists and implements necessary props/handlers
# Test 1: Find the TimesheetFilterDate component definition
echo "Searching for TimesheetFilterDate component definition..."
ast-grep --pattern 'export function TimesheetFilterDate' || ast-grep --pattern 'export const TimesheetFilterDate'
# Test 2: Check for date range state management
echo "Checking for date range state management..."
rg -A 5 "useState.*Date" apps/web/app/[locale]/timesheet/components/
Length of output: 967
Script:
#!/bin/bash
# Let's check how the date range state is being used and if it's properly connected to filtering logic
# Check the complete TimesheetFilterDate implementation
echo "Checking complete TimesheetFilterDate implementation..."
rg -A 20 "export.*TimesheetFilterDate" apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx
# Check if there are any filter-related functions or hooks that might use the date range
echo "Checking for filter-related functions using the date range..."
rg -A 10 "useTimesheet|filter.*Date|filterBy.*Date" apps/web/app/[locale]/timesheet/components/
Length of output: 1075
Script:
#!/bin/bash
# Let's check for any event emitters or context updates that might propagate the date range changes
# Check for any props, context, or event emitters in TimesheetFilter
echo "Checking TimesheetFilter for date range handling..."
rg -A 10 "TimesheetFilter" apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx
# Check for any timesheet-related context or state management
echo "Checking for timesheet context or state management..."
rg -A 10 "TimesheetContext|useTimesheet" apps/web/app/[locale]/timesheet/
Length of output: 1295
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx
Outdated
Show resolved
Hide resolved
|
@Innocent-Akim please carefully fix merge conflict! |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (2)
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx:67-79
Timestamp: 2024-10-30T16:25:19.910Z
Learning: The date range selection functionality for the date range options in the `FilterTaskActionMenu` component will be implemented in a future PR.
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx:30-35
Timestamp: 2024-10-30T16:31:01.294Z
Learning: In the `TimesheetFilter` component, the `Add Time` button does not need to use the `AddManualTimeModal` component, as per the user's decision.
🔇 Additional comments (2)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (2)
27-27: Connect TimesheetFilterDate to parent component.
As noted in previous reviews, the TimesheetFilterDate component needs to be connected to the parent component for filtering logic. Consider:
- Adding props to pass the selected date range up to the parent
- Implementing handlers in the parent to process the date range
- Connecting the date range to the actual filtering mechanism
8-17:
Review the grid layout implementation and FilterWithStatus configuration.
The grid layout might need responsive considerations for smaller screens. Additionally, the default status "Rejected" seems incorrect for initial filter state - consider using a more appropriate default like "All Tasks".
Apply this diff to fix the status:
- activeStatus="Rejected"
+ activeStatus="All Tasks"| onToggle={(label) => { | ||
| console.log(label) | ||
| }} |
There was a problem hiding this comment.
Remove console.log from production code.
The console.log statement should be replaced with proper event handling logic.
| <button | ||
| onClick={() => null} | ||
| className='flex items-center justify-center h-10 rounded-lg bg-white dark:bg-dark--theme-light border dark:border-gray-700 hover:bg-white p-3 gap-2' > | ||
| <SettingFilterIcon className="text-gray-700 dark:text-white w-3.5" strokeWidth="1.8" /> | ||
| <span className="text-gray-700 dark:text-white">Filter</span> | ||
| </button> |
There was a problem hiding this comment.
Implement onClick handler for the Filter button.
The Filter button currently has a null onClick handler. Please implement the filtering logic.
| <Button | ||
| onClick={() => null} | ||
| variant='outline' | ||
| className='bg-primary/5 dark:bg-primary-light h-10 w-[2.5rem] font-medium'> | ||
| Add Time | ||
| </Button> |
There was a problem hiding this comment.
Implement onClick handler for Add Time button.
The Add Time button currently has a null onClick handler. Based on the learnings from PR #3208, this button doesn't need to use the AddManualTimeModal component, but it still needs proper handling logic.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx (1)
135-148: Enhance accessibility of DatePickerInput.The button should include additional ARIA attributes to improve screen reader support.
const DatePickerInput: React.FC<DatePickerInputProps> = ({ date, label }) => ( <> <Button variant="outline" + aria-label={`Select ${label.toLowerCase()} date`} + role="combobox" + aria-expanded="false" className={cn( "w-[150px] justify-start text-left font-normal bg-transparent hover:bg-transparent text-black h-8 border border-transparent dark:border-transparent", !date && "text-muted-foreground" )} > {date ? format(date, "LLL dd, y") : <span>{label}</span>} </Button> - <PiCalendarDotsThin className="h-5 w-5 dark:text-gray-500" /> + <PiCalendarDotsThin className="h-5 w-5 dark:text-gray-500" aria-hidden="true" /> </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx(1 hunks)apps/web/app/[locale]/timesheet/components/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/[locale]/timesheet/components/index.tsx
🧰 Additional context used
📓 Learnings (1)
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx (1)
Learnt from: Innocent-Akim
PR: ever-co/ever-teams#3208
File: apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx:67-79
Timestamp: 2024-10-30T16:25:19.910Z
Learning: The date range selection functionality for the date range options in the `FilterTaskActionMenu` component will be implemented in a future PR.
🔇 Additional comments (1)
apps/web/app/[locale]/timesheet/components/TimesheetFilterDate.tsx (1)
1-24: LGTM! Well-structured interfaces and imports.
The TypeScript interfaces are well-defined with proper typing for all props, and the imports are organized appropriately.
| const handleToChange = (toDate: Date | null) => { | ||
| if (dateRange.from && toDate && toDate < dateRange.from) { | ||
| return; | ||
| } | ||
| setDateRange((prev) => ({ ...prev, to: toDate })); | ||
| }; |
There was a problem hiding this comment.
Add onChange callback to handleToChange.
The handleToChange function doesn't trigger the onChange callback when the 'to' date is updated, which could lead to inconsistencies in parent components.
const handleToChange = (toDate: Date | null) => {
if (dateRange.from && toDate && toDate < dateRange.from) {
return;
}
setDateRange((prev) => ({ ...prev, to: toDate }));
+ onChange?.({ ...dateRange, to: toDate });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleToChange = (toDate: Date | null) => { | |
| if (dateRange.from && toDate && toDate < dateRange.from) { | |
| return; | |
| } | |
| setDateRange((prev) => ({ ...prev, to: toDate })); | |
| }; | |
| const handleToChange = (toDate: Date | null) => { | |
| if (dateRange.from && toDate && toDate < dateRange.from) { | |
| return; | |
| } | |
| setDateRange((prev) => ({ ...prev, to: toDate })); | |
| onChange?.({ ...dateRange, to: toDate }); | |
| }; |
| const handleFromChange = (fromDate: Date | null) => { | ||
| if (maxDate && fromDate && fromDate > maxDate) { | ||
| return; | ||
| } | ||
| setDateRange((prev) => ({ ...prev, from: fromDate })); | ||
| onChange?.({ ...dateRange, from: fromDate }); | ||
| }; |
There was a problem hiding this comment.
Add user feedback for invalid date selections.
When a date selection is invalid (e.g., future date beyond maxDate), the function silently returns without providing feedback to the user.
+const [error, setError] = React.useState<string | null>(null);
const handleFromChange = (fromDate: Date | null) => {
+ setError(null);
if (maxDate && fromDate && fromDate > maxDate) {
+ setError(`Date cannot be after ${format(maxDate, 'PPP')}`);
return;
}
setDateRange((prev) => ({ ...prev, from: fromDate }));
onChange?.({ ...dateRange, from: fromDate });
};Add error display in the UI:
<PopoverContent className="w-auto p-0 flex">
+ {error && (
+ <div className="text-red-500 text-sm px-2 py-1">
+ {error}
+ </div>
+ )}
<div className="flex flex-col p-2 gap-2">Committable suggestion was skipped due to low confidence.
| const handlePresetClick = (preset: string) => { | ||
| const today = new Date(); | ||
| switch (preset) { | ||
| case 'Today': | ||
| setDateRange({ from: today, to: today }); | ||
| break; | ||
| case 'Last 7 days': | ||
| setDateRange({ | ||
| from: new Date(today.getFullYear(), today.getMonth(), today.getDate() - 7), | ||
| to: today | ||
| }); | ||
| break; | ||
| case 'Last 30 days': | ||
| setDateRange({ | ||
| from: new Date(today.getFullYear(), today.getMonth(), today.getDate() - 30), | ||
| to: today | ||
| }); | ||
| break; | ||
| case `This year (${today.getFullYear()})`: | ||
| setDateRange({ | ||
| from: new Date(today.getFullYear(), 0, 1), | ||
| to: today | ||
| }); | ||
| break; | ||
| case 'Custom Date Range': | ||
| setDateRange({ from: null, to: null }); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add onChange callback to handlePresetClick.
The handlePresetClick function updates the local state but doesn't notify parent components through the onChange callback.
const handlePresetClick = (preset: string) => {
const today = new Date();
+ let newRange: { from: Date | null; to: Date | null };
switch (preset) {
case 'Today':
- setDateRange({ from: today, to: today });
+ newRange = { from: today, to: today };
break;
case 'Last 7 days':
- setDateRange({
+ newRange = {
from: new Date(today.getFullYear(), today.getMonth(), today.getDate() - 7),
to: today
- });
+ };
break;
// ... similar changes for other cases ...
default:
+ return;
}
+ setDateRange(newRange);
+ onChange?.(newRange);
};Committable suggestion was skipped due to low confidence.
| export function DatePickerFilter({ | ||
| label, | ||
| date, | ||
| setDate, | ||
| minDate, | ||
| maxDate | ||
| }: { | ||
| label: string; | ||
| date: Date | null; | ||
| setDate: (date: Date | null) => void; | ||
| minDate?: Date | null; | ||
| maxDate?: Date | null | ||
|
|
||
| }) { | ||
| const isDateDisabled = React.useCallback((date: Date) => { | ||
| if (minDate && date < minDate) return true; | ||
| if (maxDate && date > maxDate) return true; | ||
| return false; | ||
| }, [minDate, maxDate]); | ||
|
|
||
| return ( | ||
| <div> | ||
| <DatePicker | ||
| buttonVariant={'link'} | ||
| className="dark:bg-dark--theme-light rounded-lg bg-white" | ||
| buttonClassName={'decoration-transparent flex items-center w-full bg-white dark:bg-dark--theme-light border-gray-300 justify-start text-left font-normal text-black h-10 border dark:border-slate-600 rounded-md'} | ||
| customInput={<DatePickerInput date={date} label={label} />} | ||
| mode="single" | ||
| numberOfMonths={1} | ||
| initialFocus | ||
| defaultMonth={date ?? new Date()} | ||
| selected={date ?? new Date()} | ||
| onSelect={(selectedDate) => { | ||
| if (selectedDate && !isDateDisabled(selectedDate)) { | ||
| setDate(selectedDate); | ||
| } | ||
| }} | ||
| modifiersClassNames={{ | ||
| disabled: 'text-gray-300 cursor-not-allowed', | ||
| }} | ||
| disabled={[ | ||
| ...(minDate ? [{ before: minDate }] : []), | ||
| ...(maxDate ? [{ after: maxDate }] : []) | ||
| ]} | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize DatePickerFilter performance.
Consider memoizing the custom input component to prevent unnecessary re-renders.
+const MemoizedDatePickerInput = React.memo(DatePickerInput);
export function DatePickerFilter({
label,
date,
setDate,
minDate,
maxDate
}: {
label: string;
date: Date | null;
setDate: (date: Date | null) => void;
minDate?: Date | null;
maxDate?: Date | null
}) {
const isDateDisabled = React.useCallback((date: Date) => {
if (minDate && date < minDate) return true;
if (maxDate && date > maxDate) return true;
return false;
}, [minDate, maxDate]);
+ const customInput = React.useMemo(
+ () => <MemoizedDatePickerInput date={date} label={label} />,
+ [date, label]
+ );
return (
<div>
<DatePicker
buttonVariant={'link'}
className="dark:bg-dark--theme-light rounded-lg bg-white"
buttonClassName={'decoration-transparent flex items-center w-full bg-white dark:bg-dark--theme-light border-gray-300 justify-start text-left font-normal text-black h-10 border dark:border-slate-600 rounded-md'}
- customInput={<DatePickerInput date={date} label={label} />}
+ customInput={customInput}
// ... rest of the propsCommittable suggestion was skipped due to low confidence.
|
@Innocent-Akim why NOT resolved all comments by Coderabbit? Let's either do what it suggests or Resolve (close) comments if you think it's NOT relevant (usually it is relevant from what I see so far) |
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
New Features
TimesheetFilterDate, for enhanced timesheet filtering.Improvements
TimesheetFiltercomponent to utilize a grid layout and include the new date range filter alongside existing options.