-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feat]: DataTableTimeSheet and SelectFilter components for improved and status management #3223
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,6 +26,10 @@ export function TimeSheetFilterPopover() { | |||||||||||||||||
</PopoverTrigger> | ||||||||||||||||||
<PopoverContent className="w-96"> | ||||||||||||||||||
<div className="w-full flex flex-col"> | ||||||||||||||||||
<div className="flex mb-3 text-xl font-bold gap-2"> | ||||||||||||||||||
<SettingFilterIcon className="text-gray-700 dark:text-white w-4" strokeWidth="1.8" /> | ||||||||||||||||||
<span className="text-gray-700 dark:text-white">Filters</span> | ||||||||||||||||||
</div> | ||||||||||||||||||
Comment on lines
+29
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance header accessibility. While the header layout looks good, consider improving accessibility by using semantic HTML. -<div className="flex mb-3 text-xl font-bold gap-2">
+<header className="flex mb-3 text-xl font-bold gap-2" role="banner">
<SettingFilterIcon className="text-gray-700 dark:text-white w-4" strokeWidth="1.8" />
- <span className="text-gray-700 dark:text-white">Filters</span>
+ <h2 className="text-gray-700 dark:text-white">Filters</h2>
-</div>
+</header> 📝 Committable suggestion
Suggested change
|
||||||||||||||||||
<div className="grid gap-5"> | ||||||||||||||||||
<div className=""> | ||||||||||||||||||
<label className="flex justify-between text-gray-600 mb-1 text-sm"> | ||||||||||||||||||
|
@@ -83,14 +87,14 @@ export function TimeSheetFilterPopover() { | |||||||||||||||||
triggerClassName="dark:border-gray-700" | ||||||||||||||||||
/> | ||||||||||||||||||
</div> | ||||||||||||||||||
<div className="flex items-center justify-between gap-x-4 w-full"> | ||||||||||||||||||
<div className="flex items-center justify-end gap-x-4 w-full"> | ||||||||||||||||||
<Button | ||||||||||||||||||
variant={'outline'} | ||||||||||||||||||
className='flex items-center text-sm justify-center h-10 w-full rounded-lg dark:text-gray-300' > | ||||||||||||||||||
className='flex items-center text-sm justify-center h-10 rounded-lg dark:text-gray-300' > | ||||||||||||||||||
<span className="text-sm">Clear Filter</span> | ||||||||||||||||||
</Button> | ||||||||||||||||||
<Button | ||||||||||||||||||
className='flex items-center text-sm justify-center h-10 w-full rounded-lg bg-primary dark:bg-primary-light dark:text-gray-300' > | ||||||||||||||||||
className='flex items-center text-sm justify-center h-10 rounded-lg bg-primary dark:bg-primary-light dark:text-gray-300' > | ||||||||||||||||||
<span className="text-sm">Apply Filter</span> | ||||||||||||||||||
</Button> | ||||||||||||||||||
Comment on lines
+90
to
99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve button implementation and formatting. The button implementation needs several improvements:
<div className="flex items-center justify-end gap-x-4 w-full">
<Button
variant={'outline'}
- className='flex items-center text-sm justify-center h-10 rounded-lg dark:text-gray-300' >
+ className="flex items-center text-sm justify-center h-10 rounded-lg dark:text-gray-300"
+ onClick={handleClearFilters}
+ disabled={isLoading}>
<span className="text-sm">Clear Filter</span>
</Button>
<Button
- className='flex items-center text-sm justify-center h-10 rounded-lg bg-primary dark:bg-primary-light dark:text-gray-300' >
+ className="flex items-center text-sm justify-center h-10 rounded-lg bg-primary dark:bg-primary-light dark:text-gray-300"
+ onClick={handleApplyFilters}
+ disabled={isLoading}>
+ {isLoading ? 'Applying...' : 'Apply Filter'}
</Button>
</div>
|
||||||||||||||||||
</div> | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,55 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { clsxm } from "@/app/utils"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { ReactNode } from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { FaClipboardCheck } from "react-icons/fa6"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { IoClose } from "react-icons/io5"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { RiDeleteBin6Fill } from "react-icons/ri"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type ITimesheetButton = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
title?: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onClick?: () => void, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
className?: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
icon?: ReactNode | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+6
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider improving type definitions for better type safety. The Consider this improved type definition: type ITimesheetButton = {
- title?: string,
- onClick?: () => void,
- className?: string,
- icon?: ReactNode
+ title: string,
+ icon: ReactNode,
+ onClick?: () => void,
+ className?: string
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const TimesheetButton = ({ className, icon, onClick, title }: ITimesheetButton) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<button onClick={onClick} className={clsxm("flex items-center gap-1 text-gray-400 font-normal leading-3", className)}> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<div className="w-[16px] h-[16px] text-[#293241]"> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{icon} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<span>{title}</span> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
</button> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+13
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enhance button accessibility and styling consistency. The button component lacks essential accessibility attributes and uses hardcoded color values. Apply these improvements: export const TimesheetButton = ({ className, icon, onClick, title }: ITimesheetButton) => {
return (
- <button onClick={onClick} className={clsxm("flex items-center gap-1 text-gray-400 font-normal leading-3", className)}>
- <div className="w-[16px] h-[16px] text-[#293241]">
+ <button
+ onClick={onClick}
+ aria-label={title}
+ type="button"
+ className={clsxm(
+ "flex items-center gap-1 text-gray-400 font-normal leading-3",
+ "hover:opacity-80 focus:outline-none focus:ring-2",
+ className
+ )}
+ >
+ <div className="w-4 h-4 text-gray-800">
{icon}
</div>
<span>{title}</span>
</button>
)
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export type StatusType = "Pending" | "Approved" | "Rejected"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using an enum for StatusType. Using an enum would provide better type safety and centralize the status values. -export type StatusType = "Pending" | "Approved" | "Rejected";
+export enum StatusType {
+ Pending = "Pending",
+ Approved = "Approved",
+ Rejected = "Rejected"
+} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// eslint-disable-next-line @typescript-eslint/no-empty-function | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const getTimesheetButtons = (status: StatusType) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const buttonsConfig: Record<StatusType, { icon: JSX.Element; title: string }[]> = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Pending: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ icon: <FaClipboardCheck className="!text-[#2932417c] rounded" />, title: "Approve Selected" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ icon: <IoClose className="!bg-[#2932417c] rounded" />, title: "Reject Selected" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ icon: <RiDeleteBin6Fill className="!text-[#2932417c] rounded" />, title: "Delete Selected" } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Approved: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ icon: <IoClose className="!bg-[#2932417c] rounded" />, title: "Reject Selected" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ icon: <RiDeleteBin6Fill className="!text-[#2932417c] rounded" />, title: "Delete Selected" } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Rejected: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ icon: <FaClipboardCheck className="!text-[#2932417c] rounded" />, title: "Approve Selected" }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ icon: <RiDeleteBin6Fill className="!text-[#2932417c] rounded" />, title: "Delete Selected" } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+28
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Extract button configurations to a separate constant. The button configurations should be extracted for better maintainability and reusability. Also, consider using a design system for consistent styling. Create a separate constant: export const TIMESHEET_BUTTON_CONFIGS = {
[StatusType.Pending]: [
{
id: 'approve',
icon: <FaClipboardCheck className="text-gray-600" />,
title: "Approve Selected"
},
// ... other configurations
],
// ... other status configurations
} as const; Then update the function: -const buttonsConfig: Record<StatusType, { icon: JSX.Element; title: string }[]> = {
+const buttonsConfig = TIMESHEET_BUTTON_CONFIGS; |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return (buttonsConfig[status] || buttonsConfig.Rejected).map((button, index) => ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
<TimesheetButton | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
key={index} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
icon={button.icon} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
onClick={() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// TODO: Implement the onClick functionality | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
title={button.title} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+45
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve button mapping implementation. The current implementation has several issues that need to be addressed.
Apply these improvements: - return (buttonsConfig[status] || buttonsConfig.Rejected).map((button, index) => (
+ const buttons = buttonsConfig[status] ?? buttonsConfig[StatusType.Rejected];
+ return buttons.map((button) => (
<TimesheetButton
- key={index}
+ key={button.id}
icon={button.icon}
onClick={() => {
- // TODO: Implement the onClick functionality
+ throw new Error('onClick handler not implemented');
}}
title={button.title}
/>
));
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,7 @@ import { | |||||||||||||||||||||||
} from "@components/ui/popover" | ||||||||||||||||||||||||
import { CalendarIcon } from "@radix-ui/react-icons" | ||||||||||||||||||||||||
import { format } from "date-fns" | ||||||||||||||||||||||||
import React from "react" | ||||||||||||||||||||||||
import React, { useState } from "react" | ||||||||||||||||||||||||
import { MdKeyboardArrowRight } from "react-icons/md" | ||||||||||||||||||||||||
import { PiCalendarDotsThin } from "react-icons/pi" | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -34,7 +34,7 @@ export function TimesheetFilterDate({ | |||||||||||||||||||||||
from: initialRange?.from ?? new Date(), | ||||||||||||||||||||||||
to: initialRange?.to ?? new Date(), | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const [isVisible, setIsVisible] = useState(false) | ||||||||||||||||||||||||
const handleFromChange = (fromDate: Date | null) => { | ||||||||||||||||||||||||
if (maxDate && fromDate && fromDate > maxDate) { | ||||||||||||||||||||||||
return; | ||||||||||||||||||||||||
|
@@ -92,34 +92,57 @@ export function TimesheetFilterDate({ | |||||||||||||||||||||||
aria-label="Select date range" | ||||||||||||||||||||||||
aria-expanded="false" | ||||||||||||||||||||||||
className={cn( | ||||||||||||||||||||||||
"w-[240px] justify-start text-left font-normal", | ||||||||||||||||||||||||
"w-[240px] justify-start text-left font-normal overflow-hidden text-clip", | ||||||||||||||||||||||||
!dateRange.from && "text-muted-foreground" | ||||||||||||||||||||||||
)}> | ||||||||||||||||||||||||
<CalendarIcon /> | ||||||||||||||||||||||||
{dateRange.from ? format(dateRange.from, "PPP") : <span>Pick a date</span>} | ||||||||||||||||||||||||
{dateRange.from ? ( | ||||||||||||||||||||||||
dateRange.to ? ( | ||||||||||||||||||||||||
<> | ||||||||||||||||||||||||
{format(dateRange.from, "LLL d")}-{format(dateRange.to, "d, yyyy")} | ||||||||||||||||||||||||
</> | ||||||||||||||||||||||||
) : ( | ||||||||||||||||||||||||
format(dateRange.from, "LLL d, yyyy") | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
) : ( | ||||||||||||||||||||||||
<span>Pick a date</span> | ||||||||||||||||||||||||
)} | ||||||||||||||||||||||||
</Button> | ||||||||||||||||||||||||
</PopoverTrigger> | ||||||||||||||||||||||||
<PopoverContent className="w-auto p-0 flex"> | ||||||||||||||||||||||||
<div className="flex flex-col p-2 gap-2"> | ||||||||||||||||||||||||
<DatePickerFilter | ||||||||||||||||||||||||
label="From" | ||||||||||||||||||||||||
date={dateRange.from} | ||||||||||||||||||||||||
setDate={handleFromChange} | ||||||||||||||||||||||||
/> | ||||||||||||||||||||||||
<DatePickerFilter | ||||||||||||||||||||||||
label="To" | ||||||||||||||||||||||||
date={dateRange.to} | ||||||||||||||||||||||||
setDate={handleToChange} | ||||||||||||||||||||||||
minDate={dateRange.from} | ||||||||||||||||||||||||
/> | ||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||
{isVisible && ( | ||||||||||||||||||||||||
<div className="flex flex-col p-2 gap-2 translate-x-0 justify-between"> | ||||||||||||||||||||||||
<div className="flex flex-col gap-2"> | ||||||||||||||||||||||||
<DatePickerFilter | ||||||||||||||||||||||||
label="From" | ||||||||||||||||||||||||
date={dateRange.from} | ||||||||||||||||||||||||
setDate={handleFromChange} | ||||||||||||||||||||||||
/> | ||||||||||||||||||||||||
<DatePickerFilter | ||||||||||||||||||||||||
label="To" | ||||||||||||||||||||||||
date={dateRange.to} | ||||||||||||||||||||||||
setDate={handleToChange} | ||||||||||||||||||||||||
minDate={dateRange.from} | ||||||||||||||||||||||||
/> | ||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||
<div className="flex w-full justify-end items-end"> | ||||||||||||||||||||||||
<Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Cancel</Button> | ||||||||||||||||||||||||
<Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Apply</Button> | ||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
Comment on lines
+113
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add handlers for Cancel and Apply buttons. The Cancel and Apply buttons are currently non-functional. They need onClick handlers to manage the date range selection. - <Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Cancel</Button>
- <Button variant={'outline'} className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline">Apply</Button>
+ <Button
+ variant={'outline'}
+ className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline"
+ onClick={() => {
+ setIsVisible(false);
+ setDateRange(initialRange ?? { from: new Date(), to: new Date() });
+ }}
+ >
+ Cancel
+ </Button>
+ <Button
+ variant={'outline'}
+ className="h-4 border-none text-primary hover:bg-transparent hover:text-primary hover:underline"
+ onClick={() => {
+ setIsVisible(false);
+ onChange?.(dateRange);
+ }}
+ >
+ Apply
+ </Button> Consider these improvements:
|
||||||||||||||||||||||||
<div className="border border-slate-100 my-1"></div> | ||||||||||||||||||||||||
<div className="flex flex-col p-2"> | ||||||||||||||||||||||||
{["Today", "Last 7 days", "Last 30 days", `This year (${new Date().getFullYear()})`, "Custom Date Range"].map((label, index) => ( | ||||||||||||||||||||||||
<Button | ||||||||||||||||||||||||
key={index} | ||||||||||||||||||||||||
variant="outline" | ||||||||||||||||||||||||
className="h-7 flex items-center justify-between border-none text-[12px] text-gray-700" | ||||||||||||||||||||||||
onClick={() => handlePresetClick(label)}> | ||||||||||||||||||||||||
onClick={() => { | ||||||||||||||||||||||||
label === 'Custom Date Range' && setIsVisible((prev) => !prev) | ||||||||||||||||||||||||
handlePresetClick(label) | ||||||||||||||||||||||||
}}> | ||||||||||||||||||||||||
Comment on lines
+142
to
+145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider separating concerns in the onClick handler. The current implementation combines visibility toggle and preset selection in a single line. Consider separating these for better maintainability and to prevent potential re-renders. - onClick={() => {
- label === 'Custom Date Range' && setIsVisible((prev) => !prev)
- handlePresetClick(label)
- }}
+ onClick={() => {
+ if (label === 'Custom Date Range') {
+ setIsVisible((prev) => !prev);
+ return;
+ }
+ handlePresetClick(label);
+ }} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||
<span> {label}</span> | ||||||||||||||||||||||||
{label === 'Custom Date Range' && <MdKeyboardArrowRight />} | ||||||||||||||||||||||||
</Button> | ||||||||||||||||||||||||
|
@@ -185,7 +208,9 @@ export function DatePickerFilter({ | |||||||||||||||||||||||
} | ||||||||||||||||||||||||
}} | ||||||||||||||||||||||||
modifiersClassNames={{ | ||||||||||||||||||||||||
disabled: 'text-gray-300 cursor-not-allowed', | ||||||||||||||||||||||||
disabled: 'text-[#6989AA] cursor-not-allowed', | ||||||||||||||||||||||||
selected: '!rounded-full bg-primary text-white', | ||||||||||||||||||||||||
today: '!rounded-full bg-[#BCCAD9] text-white' | ||||||||||||||||||||||||
}} | ||||||||||||||||||||||||
disabled={[ | ||||||||||||||||||||||||
...(minDate ? [{ before: minDate }] : []), | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,9 +1,10 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
import { IDailyPlan } from '@/app/interfaces' | ||||||||||||||||||||||||||||||||||||||||||||||||
import { DataTableTimeSheet } from 'lib/features/integrations/calendar' | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
export function TimesheetView() { | ||||||||||||||||||||||||||||||||||||||||||||||||
export function TimesheetView({ data }: { data?: IDailyPlan[] }) { | ||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||
<div className='grow h-full w-full bg-[#FFFFFF]'> | ||||||||||||||||||||||||||||||||||||||||||||||||
<DataTableTimeSheet /> | ||||||||||||||||||||||||||||||||||||||||||||||||
<DataTableTimeSheet data={data} /> | ||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Based on the code inspection, I can now provide the final response: Add null check for data prop in DataTableTimeSheet component The component directly uses the data prop in the table configuration without any null checks or default values. This could lead to runtime errors when data is undefined. The component should:
🔗 Analysis chainVerify DataTableTimeSheet component's handling of undefined data. Ensure that the DataTableTimeSheet component properly handles the case when data is undefined, as the type allows for this possibility. Let me run another script to check how the data is processed before being used in the table. Let me try one more search to find any data transformation or null checks. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check how DataTableTimeSheet handles undefined data prop
# Look for null checks or default values in the implementation
ast-grep --pattern 'function DataTableTimeSheet({ data }: { data?: $_[] }) {
$$$
}'
Length of output: 17661 Script: #!/bin/bash
# Check for data transformation or default value assignment before table usage
ast-grep --pattern 'const dataSourceTimeSheet = $_'
Length of output: 53 Script: #!/bin/bash
# Look for data usage and potential transformations in the component
rg -A 5 "data: " "apps/web/lib/features/integrations/calendar/table-time-sheet.tsx"
Length of output: 321 |
||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+4
to
9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider handling empty or loading states. The component doesn't handle cases where data is undefined or empty. Consider adding appropriate loading states or empty state messages for better user experience. export function TimesheetView({ data }: { data?: IDailyPlan[] }) {
return (
<div className='grow h-full w-full bg-[#FFFFFF]'>
- <DataTableTimeSheet data={data} />
+ {!data ? (
+ <div className="flex items-center justify-center h-full">
+ <p>Loading timesheet data...</p>
+ </div>
+ ) : data.length === 0 ? (
+ <div className="flex items-center justify-center h-full">
+ <p>No timesheet entries found</p>
+ </div>
+ ) : (
+ <DataTableTimeSheet data={data} />
+ )}
</div>
)
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve text color consistency and dark mode support.
The text colors are defined inconsistently:
text[#71717A]
(with incorrect syntax)text-[#7E7991]
Consider:
Also applies to: 33-33
Fix conflicting width classes.
The button has conflicting width classes:
w-full
andw-[80px]
. This can lead to inconsistent rendering across different browsers.