-
Notifications
You must be signed in to change notification settings - Fork 200
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
Implemented Date Range Input. #2328
Implemented Date Range Input. #2328
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
apps/backoffice-v2/src/common/hooks/useHomeLogic/useHomeLogic.ts
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/hooks/useHomeLogic/useHomeLogic.ts
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/hooks/useHomeLogic/useHomeLogic.ts
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/hooks/useHomeLogic/useHomeLogic.ts
Outdated
Show resolved
Hide resolved
</h3> | ||
</div> | ||
<DateRangePicker | ||
onChange={range => handleDateRangeChange(range)} |
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.
onChange={range => handleDateRangeChange(range)} | |
onChange={handleDateRangeChange} |
apps/backoffice-v2/src/common/components/organisms/Calendar/Calendar.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/organisms/Calendar/Calendar.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
useEffect(() => { | ||
if (date !== prevDateRef.current) { | ||
onChange({ | ||
start: date?.from || null, | ||
end: date?.to || null, | ||
}); | ||
prevDateRef.current = date; | ||
} | ||
}, [date, onChange]); |
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.
useEffect
should be a last resort. Here passing the date from outside as the useState
's initial value and using the onChange
on the component itself would behave in a more robust way and more consistently. Check this if you haven't yet. https://react.dev/learn/you-might-not-need-an-effect
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
apps/backoffice-v2/src/common/components/molecules/DateRangePicker/DateRangePicker.tsx
Outdated
Show resolved
Hide resolved
start: from ? new Date(from) : null, | ||
end: to ? new Date(to) : null, | ||
}} | ||
className={'<div>'} |
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.
Seems like a mistake.
const currentDate = new Date(); | ||
const [date, setDate] = useState<DateRange | undefined>({ | ||
from: new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate()), | ||
to: dayjs(new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate(), 1)) | ||
.add(1, 'month') | ||
.toDate(), | ||
}); | ||
|
||
const handleDateChange = (selection: DateRange | undefined) => { | ||
setDate(selection); | ||
onChange(selection); | ||
}; |
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.
I was expecting these lines to be removed and for the component to receive onChange and value as props. 🙂
const currentDate = new Date(); | |
const [date, setDate] = useState<DateRange | undefined>({ | |
from: new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate()), | |
to: dayjs(new Date(currentDate.getFullYear(), currentDate.getMonth(), currentDate.getDate(), 1)) | |
.add(1, 'month') | |
.toDate(), | |
}); | |
const handleDateChange = (selection: DateRange | undefined) => { | |
setDate(selection); | |
onChange(selection); | |
}; |
const handleDateRangeChange: ComponentProps<typeof DateRangePicker>['onChange'] = (range: { | ||
start: { toISOString: () => string }; | ||
end: { toISOString: () => string }; | ||
}) => { |
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.
ComponentProps['onChange'] gives you the type for range
. If the type is wrong then we're doing something incorrectly.
const handleDateRangeChange: ComponentProps<typeof DateRangePicker>['onChange'] = (range: { | |
start: { toISOString: () => string }; | |
end: { toISOString: () => string }; | |
}) => { | |
const handleDateRangeChange: ComponentProps<typeof DateRangePicker>['onChange'] = (range) => { |
|
||
export const DateRangePicker = ({ onChange, value, className }: TDateRangePickerProps) => { | ||
const [date, setDate] = useState<DateRange | undefined>(); | ||
console.log(value); |
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.
console.log(value); |
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.
I just used that for debugging, to check if the value is being updated or not. I should exclude that part, but I have made a stupid mistake. I will keep that in mind next time.
initialFocus | ||
mode="range" | ||
selected={date} | ||
onSelect={(selection: DateRange | undefined) => { |
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.
Why are you overriding the onSelect
type here?
selected={date} | ||
onSelect={(selection: DateRange | undefined) => { | ||
setDate(selection); | ||
onChange ?? onChange(selection); |
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.
You meant the opposite &&
.
onChange ?? onChange(selection); | |
onChange?.(selection); |
}; | ||
|
||
export const DateRangePicker = ({ onChange, value, className }: TDateRangePickerProps) => { | ||
const [date, setDate] = useState<DateRange | undefined>(); |
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.
Why are we not using value
over this state?
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.
The reason for not using the value prop is because the value is the initial value of the date range, whenever we select a date range in the Calendar component, the value prop is not updated automatically. Instead, we need to update a component state to reflect the new selected date range. Moreover, I tried to use the value prop but on debugging I addressed that the date range set to from and to within the value prop results in undefined just after selecting the date range, it may be an issue in development mode because of React strict mode ?
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.
The value
should update since onChange
is passed. onChange
should not be optional. If value
was used as initial I'd expect it to be passed into the useState
.
If we were to make value
optional (not saying we should) it would be fine to pass it into the useState
as well since its already initialized with undefined
.
I don't understand why we are passing value
and making it required in the interface but it doesn't do anything. I do see now its used for a conditional class.
If value
is from the URL and onChange
updates the URL I'm failing to understand why it wouldn't work. 🙂
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.
Yes, I completely understand and agree with you, the issue with the value
is that it remains undefined even after selecting the date range, due to which {!value?.from && !value?.to && <span>Pick a date</span>}
always remains active even after selecting the date range.
And as the type of onChange
is SelectRangeEventHandler | undefined
here, we cannot invoke an object that is possibly undefined
thus need to use optional chaining.
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.
Addressed in 38644b7
const [{ from, to }, setSearchParams] = useZodSearchParams(HomeSearchSchema); | ||
const { data: session } = useAuthenticatedUserQuery(); | ||
const { firstName, fullName, avatarUrl } = session?.user || {}; | ||
const queryParams = from || to ? `?from=${from}&to=${to}` : ''; |
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.
This one is redundant. Overtime search query params will change, might as well pass the full search
prop from useLocation
like done in Cases.Item.tsx
.
<div> | ||
<Tabs defaultValue={pathname} key={pathname}> | ||
<TabsList> | ||
<TabsTrigger asChild={true} value={`/${locale}/home/statistics`}> |
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.
Is the active tab state affected by the search query param change? Do we need to also pass search
into value
?
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.
Addressed in 6f4033d
* Added: Navigation and Routing (#2281) * Added: Navigation and Routing * Changes: Added to set the Home nav-item as active when it is in active state * Changes: Updated the files with suggested code * Changes: Revert the previous changes as the code was not required. * Added: Welcome component (#2290) * Added: Welcome component * Updated: Files with suggested changes * Updated: Files with suggested changes * Updated: Files with suggested changes * Changes: To handle the fallback * refactor(backoffice-v2): updated based on dev changes --------- Co-authored-by: Omri Levy <[email protected]> * Added: ShadCN Tabs Component with React Router NavLink (#2298) * fix: fix ongoing report generations (#2295) * Feature/implement error handling for specific product (#2296) * fix: fix ongoing report generations * feat: added failure invokeOngoingAuditReport handling over specific business * reverted cron to EVERY_DAY_AT_MIDNIGHT * Added: Tabs component * Changes: Updated the files with required changes * Changes: Applied to check for user authentication and current path * Changes: Removed authentication check from Home; as it was not required * Changes: Removed authentication check from Home; as it was not required * Updated: Removed instance of isauthenticated --------- Co-authored-by: Daniel Blokh <[email protected]> * Updated active link handling (#2310) * Added: Tabs component * Changes: Updated the files with required changes * Changes: Applied to check for user authentication and current path * Changes: Removed authentication check from Home; as it was not required * Changes: Removed authentication check from Home; as it was not required * Updated: Removed instance of isauthenticated * fix(backoffice-v2): active link handling * refactor(*): fixed incorrect changes --------- Co-authored-by: Shashank Vishwakarma <[email protected]> * Implemented Date Range Filtering with Zod (#2312) * Added: Date range * Updated: file with required changes * Changes: updated files with suggested changes * Changes: Applied to place the schema file under Home and appropriate naming convention. * Implemented Date Range Input. (#2328) * Added: date range input component using ShadCN's date picker component * Changes: applied to necessary files with suggested code * Changes: Applied to take a try with just onChange as a prop. * Changes: Updated the files with necessary props * Updated: Applied suggested changes * fix(backoffice-v2): addressed pr comments * refactor(backoffice-v2): simplified undefined values * Changes: Applied to seperate the business logic annd presentation data within home-page * Applied: Suggested changes to useHomeLogic * Updated: Fixed the active state of tabs using defaultTabValue * Changes: Applied suggested file changes * updated useHomeLogic exports --------- Co-authored-by: Omri Levy <[email protected]> * Statistics page (#2471) * feat(backoffice-v2): initial user stats section * feat(backoffice-v2): initial portfolio risk stats section * feat(backoffice-v2): initial workflow stats section * refactor(backoffice-v2): moves statistics sections to separate modules * refactor(backoffice-v2): all mocks now use variables and iterations * refactor(backoffice-v2): updated mock values * refactor(backoffice-v2): removed assignColorToItem * refactor(backoffice-v2): removed value-to-color * refactor(backoffice-v2): addressed pr comments * refactor(backoffice-v2): addressed pr comment --------- Co-authored-by: Shashank Vishwakarma <[email protected]> Co-authored-by: Daniel Blokh <[email protected]> Co-authored-by: Alon Peretz <[email protected]>
Description
Added a date range input component using shadcn/ui date picker component and Calendar component to select the dates.
Related issues
Fixes: #2270
Breaking changes