Feat: Implement Timesheet Filter Component With Status, Date Filters #3208
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve various components within the web application, primarily focusing on formatting, layout adjustments, and the introduction of new functionality. Key modifications include the addition of a 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (16)
apps/web/app/[locale]/timesheet/components/FilterWithStatus.tsx (2)
Line range hint 15-22: Hardcoded counts should be dynamic
The buttonData array contains hardcoded counts which should ideally be derived from actual data.
Consider modifying the component to accept counts as props:
type FilterCounts = {
allTasks: number;
pending: number;
approved: number;
rejected: number;
}
interface FilterWithStatusProps {
activeStatus: FilterStatus;
onToggle: (status: FilterStatus) => void;
counts: FilterCounts;
className?: HTMLAttributes<HTMLDivElement>;
}Line range hint 1-34: Consider accessibility improvements
The component could benefit from some accessibility enhancements:
- Missing ARIA labels for the filter buttons
- No keyboard navigation support for the filter group
Consider these improvements:
<div
+ role="radiogroup"
+ aria-label="Timesheet status filter"
className={clsxm('flex flex-nowrap h-[2.2rem] items-center bg-[#e2e8f0aa] rounded-xl w-full', className)}>
{buttonData.map(({ label, count, icon }, index) => (
<Button
key={index}
+ role="radio"
+ aria-checked={activeStatus === label}
className={/*...*/}
onClick={() => onToggle(label)}>apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (1)
22-30: Consider adding loading and error states
The filter components lack loading and error states, which could lead to poor user experience when data is being fetched or when errors occur.
Consider:
- Adding loading spinners during data fetching
- Implementing error boundaries or error states
- Disabling filters when data is not available
apps/web/app/[locale]/timesheet/components/TimesheetCard.tsx (2)
Line range hint 24-63: Consider enhancing accessibility for interactive elements
While the card has good accessibility features like aria-label on the parent and button, the icon section might need improvement:
- Using
aria-hidden="true"on the icon container is correct if it's purely decorative - However, if the entire card is clickable (since there's an
onClickprop), we should ensure proper keyboard navigation
Consider these improvements:
<Card
aria-label={`Timesheet card for ${title}`}
+ role="article"
+ tabIndex={onClick ? 0 : undefined}
+ onKeyDown={(e) => {
+ if (onClick && (e.key === 'Enter' || e.key === ' ')) {
+ e.preventDefault();
+ onClick();
+ }
+ }}
shadow='custom'
- className='w-full h-[175px] rounded-md border border-gray-200 flex gap-8 shadow shadow-gray-100 p-3'>
+ className={clsxm(
+ 'w-full h-[175px] rounded-md border border-gray-200 flex gap-8 shadow shadow-gray-100 p-3',
+ onClick && 'cursor-pointer hover:border-gray-300'
+ )}>Line range hint 28-29: Clean up class string formatting
There's an extra space in the flex class string: flex gap-8
- className='w-full h-[175px] rounded-md border border-gray-200 flex gap-8 shadow shadow-gray-100 p-3'>
+ className='w-full h-[175px] rounded-md border border-gray-200 flex gap-8 shadow shadow-gray-100 p-3'>apps/web/app/[locale]/timesheet/components/time-sheet-filter-popover.tsx (1)
1-103: Consider implementing filter persistence.
The current implementation doesn't persist filter selections across sessions or page reloads.
Consider these improvements:
- Use URL query parameters to persist filter selections
- Implement a filter context to share filter state across components
- Add local storage backup for filter preferences
Would you like me to provide implementation examples for any of these approaches?
apps/web/app/[locale]/timesheet/page.tsx (1)
Line range hint 63-64: Address hardcoded values and improve type safety.
Several improvements could enhance the code quality:
- The greeting message contains a hardcoded name "Ruslan". Consider using the authenticated user's name.
- Add proper aria-labels to interactive elements for better accessibility.
- Consider adding TypeScript types for the card metrics (count, hours) to ensure type safety.
Example implementation for the greeting:
- <h1 className='!text-[23px] font-bold text-[#282048]'>Good morning, Ruslan !</h1>
+ <h1 className='!text-[23px] font-bold text-[#282048]'>
+ Good morning, {user?.firstName || 'Guest'}!
+ </h1>apps/web/lib/features/integrations/calendar/setup-full-calendar.tsx (7)
Line range hint 39-67: Replace hardcoded events with dynamic data.
The events array contains hardcoded dates set to August 2024. This could cause issues as these events might not be visible in the current calendar view.
Consider fetching events dynamically or using current date-relative values for testing:
- start: '2024-08-01',
+ start: new Date().toISOString().split('T')[0],Line range hint 91-111: Replace 'any' types with proper TypeScript interfaces.
Several functions use the any type, which defeats TypeScript's type checking benefits.
Define proper interfaces for the calendar event info:
interface CalendarDayCellInfo {
date: Date;
}
interface CalendarEventInfo {
event: {
title: string;
extendedProps: {
icon: JSX.Element;
};
};
}Line range hint 113-121: Remove commented code.
There's commented out code for handleEventClick that should be either implemented or removed.
Remove the commented code block if it's not needed, or implement the functionality if it's required.
Line range hint 159-185: Remove debug styling and implement proper data flow.
The component has several issues:
- Debug styling (
bg-red-400) is present - Component contains hardcoded data instead of receiving it through props
Consider these improvements:
- <div className='flex flex-col w-full bg-red-400 md:h-[50vh] lg:h-[90vh] xl:h-[95vh] sticky top-0'>
+ <div className='flex flex-col w-full md:h-[50vh] lg:h-[90vh] xl:h-[95vh] sticky top-0'>
- <CardItemsProjects logo="https://dummyimage.com/330x300/0ecc9D/ffffff.jpg&text=N" title='Ever teams projects' totalHours='Total hours 52h' />
+ <CardItemsProjects projects={selectedDateProjects} />Line range hint 187-203: Improve image handling and accessibility.
The component needs improvements in error handling and accessibility:
- Missing fallback for image loading errors
- Interactive element lacks keyboard accessibility
Apply these improvements:
- <div className='w-full flex items-center space-x-2 p-1 cursor-pointer hover:bg-gray-100 rounded dark:hover:bg-gray-700'>
+ <button
+ className='w-full flex items-center space-x-2 p-1 hover:bg-gray-100 rounded dark:hover:bg-gray-700'
+ onClick={() => onMemberClick?.(name)}
+ aria-label={`View ${name}'s details`}
+ >
<Image
className='text-white p-1 rounded-full flex items-center justify-center h-8 w-8'
src={imageUrl ? imageUrl : ''}
+ alt={`${name}'s avatar`}
+ onError={(e) => {
+ e.currentTarget.src = '/default-avatar.png'
+ }}
width={90}
height={90}
/>Line range hint 205-221: Add ARIA labels and improve image handling.
The component needs accessibility improvements and better image error handling.
Apply these improvements:
- <div className='flex items-center justify-start space-x-2 w-full pb-2'>
+ <button
+ className='flex items-center justify-start space-x-2 w-full pb-2'
+ aria-expanded="false"
+ aria-label={`Toggle ${title} details`}
+ >
<Image
src={logo ? logo : ''}
alt='logos'
+ alt={`${title} logo`}
+ onError={(e) => {
+ e.currentTarget.src = '/default-project-logo.png'
+ }}
width={100}
height={100}
className='h-8 w-8 bg-cover rounded-lg flex items-center justify-center'
/>Line range hint 224-233: Make total hours configurable via props.
The component uses a hardcoded value for total hours instead of receiving it as a prop.
Implement proper prop handling:
- export function TotalHours() {
+ export function TotalHours({ total }: { total: number }) {
return (
<div className="w-[200px] flex items-center !text-gray-800 dark:!text-slate-200 justify-between text-left font-normal h-10 border border-slate-200 rounded-lg px-2">
<div className="flex items-center">
<IoTimeSharp className="mr-2 h-5 w-5" />
- <span>Total Hours 240</span>
+ <span>Total Hours {total}</span>
</div>
</div>
)
}apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx (2)
38-38: Consider using relative units for width
Using fixed pixel values like w-[170px] may affect responsiveness on different screen sizes. Consider using relative units (e.g., w-full, max-w-sm) or responsive design utilities to enhance adaptability.
90-90: Initialize 'isDropdownOpen' to false for expected behavior
Setting isDropdownOpen to true will make the CustomDateRange dropdown open by default, which may not be intended.
Initialize the state as:
- const [isDropdownOpen, setDropdownOpen] = React.useState(true);
+ const [isDropdownOpen, setDropdownOpen] = React.useState(false);📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- apps/web/app/[locale]/calendar/component.tsx (1 hunks)
- apps/web/app/[locale]/timesheet/components/FilterWithStatus.tsx (1 hunks)
- apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx (4 hunks)
- apps/web/app/[locale]/timesheet/components/TimesheetCard.tsx (2 hunks)
- apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (1 hunks)
- apps/web/app/[locale]/timesheet/components/TimesheetView.tsx (0 hunks)
- apps/web/app/[locale]/timesheet/components/time-sheet-filter-popover.tsx (1 hunks)
- apps/web/app/[locale]/timesheet/page.tsx (3 hunks)
- apps/web/lib/features/integrations/calendar/setup-full-calendar.tsx (1 hunks)
- apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/[locale]/timesheet/components/TimesheetView.tsx
✅ Files skipped from review due to trivial changes (2)
- apps/web/app/[locale]/calendar/component.tsx
- apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (9)
apps/web/app/[locale]/timesheet/components/FilterWithStatus.tsx (1)
23-23: LGTM: Layout change improves flexibility
The switch from grid to flex layout with flex-nowrap is a good choice as it ensures buttons stay in a single row and maintain consistent spacing.
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (2)
1-5: LGTM: Imports are well-organized
The imports are properly structured, separating local components from external library imports.
7-42: Verify integration with parent components
The TimesheetFilter component contains multiple filter sub-components that might affect the parent component's state and performance.
Let's check the component's usage and data flow:
#!/bin/bash
# Search for TimesheetFilter usage
rg -A 5 "TimesheetFilter" --type tsx
# Check for related state management
ast-grep --pattern 'const [$_] = useState<{ status?: string, date?: $_}>($$$)'apps/web/app/[locale]/timesheet/components/TimesheetCard.tsx (1)
Line range hint 7-64: Overall implementation looks good!
The TimesheetCard component is well-structured with:
- Clear prop interface
- Good use of semantic HTML
- Responsive text sizing
- Proper event handling
apps/web/app/[locale]/timesheet/components/time-sheet-filter-popover.tsx (2)
1-11: LGTM! Clean import organization.
The imports are well-organized and properly grouped by their sources.
1-103:
Missing date filter implementation.
According to the PR objectives, the component should include date filters, but they are not implemented.
Let's verify if date filters are implemented elsewhere:
Please add the date filter section to meet the PR requirements:
<div className="">
<label className="flex justify-between text-gray-600 mb-1 text-sm">
<span className="text-[12px]">Date Range</span>
<span className="text-primary/10">Clear</span>
</label>
<DynamicDatePicker
onChange={handleDateRangeChange}
value={dateRange}
selectsRange
startDate={dateRange.start}
endDate={dateRange.end}
/>
</div>apps/web/app/[locale]/timesheet/page.tsx (1)
74-74: Verify icon sizing implementation.
The removal of explicit size classes from icons suggests that sizing is now handled elsewhere. Please ensure this aligns with the design system and that icons maintain consistent sizing across different screen sizes.
Also applies to: 81-81, 88-88
apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx (2)
21-24: Imports are appropriate for new functionality
The added imports (DatePicker, cn, format, PiCalendarDotsThin) are necessary for the new components and features introduced.
170-170: Ensure 'DatePicker' handles null 'selected' value
The selected prop is set using date!, asserting that date is not null. Since date can be null, ensure that the DatePicker component gracefully handles a null value for selected.
Please confirm that DatePicker accepts null or undefined for the selected prop without errors. If not, consider providing a default value or adjusting the component to handle null.
apps/web/app/[locale]/timesheet/components/FilterWithStatus.tsx
Outdated
Show resolved
Hide resolved
| <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.
💡 Codebase verification
Based on the search results, I can see that there's an existing implementation of the Add Time functionality in the codebase. The AddManualTimeModal component from lib/features/manual-time/add-manual-time-modal.tsx is used for this purpose, and it's already being used in other parts of the application.
The Add Time button should use the existing AddManualTimeModal component
The button's onClick handler should open the AddManualTimeModal component. Here are the specific changes needed:
- Import the AddManualTimeModal component from 'lib/features/manual-time/add-manual-time-modal'
- Add state management for the modal's open/close status
- Connect the button's onClick handler to open the modal
Example implementation found in apps/web/lib/features/task/task-filters.tsx:
<AddManualTimeModal closeModal={closeManualTimeModal} isOpen={isManualTimeModalOpen} params="AddTime" />🔗 Analysis chain
Implement Add Time button functionality
The Add Time button has an empty onClick handler. This seems to be incomplete implementation.
Let's check if there are any existing implementations we should use:
Would you like me to help implement the Add Time functionality or create a GitHub issue to track this task?
Let me search for implementations using different patterns to ensure we don't miss anything. The previous command failed because of incorrect file type specification, so I've corrected that and added more comprehensive searches.
Let me try different search patterns and commands since the previous ones had issues with file type recognition. I'll also check the timesheet directory structure to understand the context better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing Add Time implementations
rg -i "add.*time" --type tsx
Length of output: 247
Script:
#!/bin/bash
# Search for existing Add Time implementations with ripgrep, using correct file type
rg -i "add.*time" -t ts -t tsx
# Look for any modal or form components related to time entry
rg -i "(timeentry|addtime|timeinput)" -t ts -t tsx
# Search for any state management or handlers related to time
ast-grep --pattern 'const [$_] = useState($$$)'
ast-grep --pattern 'function handle[$_]Time($$$) { $$$ }'
Length of output: 697
Script:
#!/bin/bash
# Try searching with just 't' type for TypeScript files
rg -i "add.*time" -t ts
rg -i "(timesheet|timelog)" -t ts
# Look for any timesheet-related components or handlers
fd -e tsx -e ts | grep -i "time"
# Check the specific directory for related components
ls -R apps/web/app/*/timesheet/
Length of output: 39438
There was a problem hiding this comment.
No changes here
There was a problem hiding this comment.
@Innocent-Akim, understood.
(A small rabbit looks down sadly.)
✏️ Learnings added
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:00.562Z
Learning: In the `TimesheetFilter` component, the `Add Time` button does not need to use the `AddManualTimeModal` component, as per the user's decision.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| <DropdownMenuItem onClick={(e) => e.preventDefault()} className="cursor-pointer hover:bg-primary" > | ||
| Today | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem className="cursor-pointer" > | ||
| <DropdownMenuItem onClick={(e) => e.preventDefault()} className="cursor-pointer hover:bg-primary" > | ||
| Last 7 days | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem className="cursor-pointer" > | ||
| <DropdownMenuItem onClick={(e) => e.preventDefault()} className="cursor-pointer hover:bg-primary" > | ||
| Last 30 days | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem className="cursor-pointer" > | ||
| This year (2024) | ||
| {/* ({new Date().getFullYear()}) */} | ||
| <DropdownMenuItem onClick={(e) => e.preventDefault()} className="cursor-pointer hover:bg-primary" > | ||
| This year ({new Date().getFullYear()}) | ||
| </DropdownMenuItem> | ||
| {/* <DropdownMenuSeparator /> */} | ||
| <CustomDateRange /> |
There was a problem hiding this comment.
Implement functionality for date range selection
The onClick handlers for the date range options currently prevent default behavior but do not update any state. To make the menu functional, these handlers should update the component's state to reflect the selected date range.
For example:
<DropdownMenuItem
- onClick={(e) => e.preventDefault()}
+ onClick={() => handleDateRangeSelect('today')}
className="cursor-pointer hover:bg-primary" >
Today
</DropdownMenuItem>Define the handleDateRangeSelect function to update the state based on the selected range.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
No changes here because I’ll add some modifications in the last PR.
There was a problem hiding this comment.
@Innocent-Akim, understood. Thank you for letting me know. I'll note that you'll be adding these modifications in a future PR.
(_/)
(•‿•)
( 📝)
✏️ Learnings added
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.660Z
Learning: The date range selection functionality for the date range options in the `FilterTaskActionMenu` component will be implemented in a future PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| <span className="">20-10-20</span> | ||
| </Button> |
There was a problem hiding this comment.
Replace hard-coded date with dynamic value
The date displayed is hard-coded as "20-10-20". This should be dynamic to reflect the selected date range or the current date for better user experience.
Consider updating the code to use a state variable that holds the selected date:
- <span className="">20-10-20</span>
+ <span className="">{selectedDate ? format(selectedDate, 'dd-MM-yy') : 'Select Date'}</span>Ensure selectedDate is managed via React.useState and updated appropriately.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
No changes here because I’ll add some modifications in the last PR.
There was a problem hiding this comment.
@Innocent-Akim, understood.
(_/)
(..)
(")(")
🧠 Learnings used
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.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/web/app/[locale]/timesheet/components/FilterWithStatus.tsx (1 hunks)
- apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx (4 hunks)
- apps/web/app/[locale]/timesheet/components/TimesheetCard.tsx (2 hunks)
- apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/[locale]/timesheet/components/FilterWithStatus.tsx
- apps/web/app/[locale]/timesheet/components/TimesheetCard.tsx
🔇 Additional comments (2)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (2)
11-12: LGTM! Responsive layout implemented correctly.
The layout implementation follows best practices with:
- Flexible container using flex-col/flex-row for responsive design
- Appropriate breakpoints for mobile adaptation
- Proper spacing using gap utilities
15-17: Implement the filter toggle handler and add type safety.
The TODO comment should be addressed with proper implementation. Consider:
- Adding proper TypeScript types for the label parameter
- Implementing the toggle handler to update the filter state
Would you like help implementing the toggle handler with proper type safety?
| @@ -30,7 +35,7 @@ | |||
| <Select | |||
| value={selectedValue} | |||
| onValueChange={handleSelectChange}> | |||
| <SelectTrigger className="w-[180px] border border-gray-200 dark:border-gray-700 bg-white dark:bg-dark--theme-light focus:ring-2 focus:ring-transparent"> | |||
| <SelectTrigger className="w-[170px] border border-gray-200 dark:border-gray-700 bg-white dark:bg-dark--theme-light focus:ring-2 focus:ring-transparent"> | |||
There was a problem hiding this comment.
💡 Codebase verification
Width change appears inconsistent with similar components
The width reduction from 180px to 170px creates an inconsistency with the calendar component that uses the same select pattern. The calendar component at apps/web/app/[locale]/calendar/component.tsx maintains a width of 180px for its SelectTrigger, which suggests this width reduction might be unintentional.
calendar/component.tsx: Usesw-[180px]timesheet/components/FrequencySelect.tsx: Changed tow-[170px]
Both components appear to serve similar purposes with the same placeholder "Select a daily", so they should maintain consistent widths for a uniform UI experience.
🔗 Analysis chain
Verify the impact of width reduction.
The SelectTrigger width has been reduced from 180px to 170px. Please ensure this change doesn't cause content truncation or layout shifts in the UI.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of similar width classes to verify consistency
rg -l 'w-\[1[7-8]0px\]' --type tsx
Length of output: 64
Script:
#!/bin/bash
# Search for width classes with proper file type
rg -l 'w-\[1[7-8]0px\]' --type ts --type tsx --type jsx --type js
# Also search for SelectTrigger components to understand the width patterns
rg -l 'SelectTrigger' --type ts --type tsx --type jsx --type js
Length of output: 187
Script:
#!/bin/bash
# Search for width classes without file type restrictions
rg -l 'w-\[1[7-8]0px\]'
# Search for SelectTrigger components without file type restrictions
rg 'SelectTrigger.*w-\[' -A 1 -B 1
Length of output: 2085
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (1)
7-45: Consider implementing centralized filter state management.The component handles multiple filter types (status, frequency, task filters). Consider implementing a centralized state management solution (e.g., React Context, Zustand, or Redux) to:
- Manage filter states cohesively
- Share filter states across components
- Implement filter reset functionality
- Persist filter preferences
This would make the component more maintainable and easier to extend with future filter requirements.
apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx (1)
113-134: Enhance accessibility of the date range selector.Consider the following accessibility improvements:
- Add aria-label to buttons for better screen reader support
- Include role="dialog" and aria-labelledby attributes on the date range selector
Apply these improvements:
-<button className="text-primary" onClick={cancelChanges}>Cancel</button> -<button className="text-primary" onClick={applyChanges}>Apply</button> +<button + className="text-primary" + onClick={cancelChanges} + aria-label="Cancel date range selection" +> + Cancel +</button> +<button + className="text-primary" + onClick={applyChanges} + aria-label="Apply date range selection" +> + Apply +</button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx(4 hunks)apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (2)
apps/web/app/[locale]/timesheet/components/FrequencySelect.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.
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/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.
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 (5)
apps/web/app/[locale]/timesheet/components/TimesheetFilter.tsx (3)
11-12: LGTM! Responsive layout implementation.
The flex layout with responsive breakpoints is well-implemented, providing a good mobile-first approach with the flex-col md:flex-row classes.
35-40:
Implement the Add Time button handler.
The onClick handler is currently set to null. Please implement the appropriate handler for the Add Time functionality.
Let's check for existing Add Time implementations that don't use AddManualTimeModal:
#!/bin/bash
# Search for Add Time button implementations
rg -A 5 "onClick.*[Aa]dd[Tt]ime"13-18:
Implement the filter toggle handler.
The onToggle handler contains a TODO comment. Please implement the handler to manage filter state changes.
Let's check for any existing filter state management implementations:
apps/web/app/[locale]/timesheet/components/FrequencySelect.tsx (2)
Line range hint 9-24: LGTM! Well-organized imports and interface definition.
The new imports are properly organized and the DatePickerInputProps interface is well-defined with clear type definitions.
Also applies to: 26-29
143-156: LGTM! Well-structured presentational component.
The DatePickerInput component is well-implemented with proper typing and conditional rendering.
| export function DynamicDatePicker({ | ||
| label, | ||
| date, | ||
| setDate, | ||
| }: { | ||
| label: string; | ||
| date: Date | null; | ||
| setDate: (date: Date | null) => void; | ||
| }) { | ||
| 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) => selectedDate && setDate(selectedDate)} | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error boundary protection.
The DynamicDatePicker component handles dates well but could benefit from error boundary protection for robust error handling.
Consider wrapping the component with an error boundary:
class DatePickerErrorBoundary extends React.Component {
state = { hasError: false };
static getDerivedStateFromError() {
return { hasError: true };
}
render() {
if (this.state.hasError) {
return <div>Error loading date picker. Please try again.</div>;
}
return this.props.children;
}
}
// Usage
export function DynamicDatePicker({...props}) {
return (
<DatePickerErrorBoundary>
<div>
<DatePicker {...existing props} />
</div>
</DatePickerErrorBoundary>
);
}There was a problem hiding this comment.
@Innocent-Akim, understood, I'll skip this suggestion.
(_/)
(;_;)
🧠 Learnings used
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.
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
Release Notes
New Features
DynamicDatePickerfor enhanced date selection.TimeSheetFilterPopovercomponent for filtering tasks by various criteria.Improvements
FilterWithStatus,TimesheetCard, andTimesheetFiltercomponents for a more compact design.TimesheetPagecomponent.DataTableTimeSheetand calendar components.TimesheetFilterwith new filtering options.Bug Fixes