Added date-field reusable component; Migrated old DateFormFields to new UI components
#10049
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes to date input handling across multiple components. The primary focus is on replacing the deprecated Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/pages/PublicAppointments/PatientRegistration.tsx (1)
Line range hint
89-89: Improve date validation in the schema.The current schema validation for
date_of_birthneeds improvement:
- Remove the string type to maintain type consistency.
- Add validation for minimum date.
Apply this diff to improve the schema:
-date_of_birth: z.date().or(z.string()).optional(), +date_of_birth: z.date() + .optional() + .refine( + (date) => { + if (!date) return true; + const minDate = new Date(); + minDate.setFullYear(minDate.getFullYear() - 150); + return date >= minDate && date <= new Date(); + }, + { message: t("invalid_date_range") } + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/pages/PublicAppointments/PatientRegistration.tsx (2)
11-11: LGTM!The import statement for the new DatePicker component follows the project's conventions.
Line range hint
193-193: Verify date formatting in form submission.The
dateQueryStringutility is used to format the date, but there's no validation to ensure the date is properly formatted.Run this script to verify the implementation of
dateQueryString:
| <DatePicker | ||
| date={ | ||
| field.value ? new Date(field.value) : undefined | ||
| } | ||
| onChange={(dateObj: { | ||
| name: string; | ||
| value: Date; | ||
| }) => { | ||
| if (dateObj?.value instanceof Date) { | ||
| field.onChange(dateObj.value.toISOString()); | ||
| } else { | ||
| field.onChange(null); | ||
| } | ||
| }} | ||
| disableFuture | ||
| min={new Date(1900, 0, 1)} | ||
| className="-mb-6" | ||
| onChange={(date) => field.onChange(date)} |
There was a problem hiding this comment.
Add date validation and improve type safety.
The DatePicker implementation needs several improvements:
- Add validation to prevent future dates (previously handled by
disableFuture). - Add type validation for the date conversion.
- Sanitize the date input before form mutation.
- Add accessibility attributes.
Apply this diff to address these issues:
<DatePicker
- date={field.value ? new Date(field.value) : undefined}
- onChange={(date) => field.onChange(date)}
+ date={field.value ? new Date(field.value) : undefined}
+ onChange={(date) => {
+ if (date && date > new Date()) {
+ return;
+ }
+ field.onChange(date);
+ }}
+ disableFuture
+ aria-label={t("date_of_birth")}
/>Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/ui/date-picker.tsx (1)
48-48: Consider adding aria-label for disabled dates.The disabled prop is correctly passed to the Calendar component, but for better accessibility, consider adding an aria-label to explain why certain dates are disabled.
<Calendar mode="single" selected={date} onSelect={(date) => { onChange?.(date); setOpen(false); }} initialFocus - disabled={disabled} + disabled={disabled} + disabledDayAriaLabel="This date is not available for selection" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/date-picker.tsx(2 hunks)src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/PublicAppointments/PatientRegistration.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/components/ui/date-picker.tsx (2)
19-19: LGTM! Well-typed disabled prop.The disabled prop is correctly typed as an optional function that takes a Date and returns a boolean, following TypeScript best practices.
Line range hint
22-51: Verify date handling in patient registration.Since this component is replacing DateFormField in the patient registration process, ensure that:
- The date format matches the expected format in the backend API
- The disabled function correctly prevents selection of future dates for date of birth
- The component handles null/undefined values gracefully
✅ Verification successful
Date handling implementation verified successfully
The DatePicker component in the patient registration process correctly:
- Prevents selection of future dates and dates before 1900
- Handles null/undefined values with a "Pick a date" placeholder
- Uses standard Date objects for API compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the date format and validation in the patient registration component rg -A 10 "DatePicker.*date.*birth" --type ts --type tsx # Check for any date format constants or utilities rg "date.*format" --type ts --type tsxLength of output: 154
Script:
#!/bin/bash # Find DatePicker usage in components ast-grep --pattern 'DatePicker date={$_} onChange={$_}' # Find patient registration form components rg -l "registration.*form|form.*registration" # Look for Calendar component implementation ast-grep --pattern 'Calendar mode="single" $_' # Search for API calls with dates rg -l "api.*patient|patient.*api"Length of output: 444
Script:
#!/bin/bash # Find DatePicker usage with broader pattern rg -l "DatePicker" # Check date formats in API files rg -A 5 "date.*birth|birth.*date" src/types/**/*Api.ts src/Utils/request/api.tsx # Find Calendar component usage and implementation rg -l "Calendar.*mode|mode.*Calendar" # Look for date-related types rg -A 3 "interface.*date|type.*date" src/types/Length of output: 1471
Script:
#!/bin/bash # Check PatientRegistration implementation rg -A 10 "DatePicker" src/pages/PublicAppointments/PatientRegistration.tsx # Look for birth date validation rg -A 5 "birth.*valid|valid.*birth" # Check patient API request formatting rg -A 10 "patient.*create|create.*patient"Length of output: 22492
|
@NikhilA8606 Is this work in progress 🤔 Only seeing DateFormField here. Or are you planning for new PRs for each component? |
|
I'm currently working on this PR |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/ui/datefield.tsx (1)
6-9: Consider adding validation props for date constraints.The component could benefit from additional props to handle date validation constraints (min/max date, disabled dates, etc.).
interface DateFieldProps { value?: string; onChange?: (value: string) => void; + minDate?: string; + maxDate?: string; + disabledDates?: string[]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/PatientRegistration.tsx(2 hunks)src/components/ui/datefield.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Patient/PatientRegistration.tsx (1)
516-519: LGTM! Clean integration of the DateField component.The DateField component is properly integrated with form control and handles value/onChange correctly.
src/components/ui/datefield.tsx
Outdated
| <Input | ||
| type="number" | ||
| placeholder="DD" | ||
| value={day} | ||
| onChange={handleDayChange} | ||
| data-cy="dob-day-input" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add min/max attributes to input fields.
The numeric inputs should have min/max constraints to prevent invalid values.
<Input
type="number"
placeholder="DD"
+ min="1"
+ max="31"
value={day}
onChange={handleDayChange}
data-cy="dob-day-input"
/>
<Input
type="number"
placeholder="MM"
+ min="1"
+ max="12"
value={month}
onChange={handleMonthChange}
data-cy="dob-month-input"
/>
<Input
type="number"
placeholder="YYYY"
+ min="1900"
+ max={new Date().getFullYear()}
value={year}
onChange={handleYearChange}
data-cy="dob-year-input"
/>Also applies to: 51-57, 62-68
src/components/ui/datefield.tsx
Outdated
| const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newDay = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${year || ""}-${month || ""}-${newDay}`); | ||
| } | ||
| }; | ||
|
|
||
| const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newMonth = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${year || ""}-${newMonth}-${day || ""}`); | ||
| } | ||
| }; | ||
|
|
||
| const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newYear = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${newYear}-${month || ""}-${day || ""}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add date validation in the handlers.
The handlers should validate the input values to ensure they form a valid date (e.g., prevent 31st for months with 30 days).
const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newDay = e.target.value;
+ if (Number(newDay) > 31 || Number(newDay) < 1) return;
if (onChange) {
onChange(`${year || ""}-${month || ""}-${newDay}`);
}
};
const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newMonth = e.target.value;
+ if (Number(newMonth) > 12 || Number(newMonth) < 1) return;
if (onChange) {
onChange(`${year || ""}-${newMonth}-${day || ""}`);
}
};
const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newYear = e.target.value;
+ if (Number(newYear) < 1900 || Number(newYear) > new Date().getFullYear()) return;
if (onChange) {
onChange(`${newYear}-${month || ""}-${day || ""}`);
}
};📝 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 handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newDay = e.target.value; | |
| if (onChange) { | |
| onChange(`${year || ""}-${month || ""}-${newDay}`); | |
| } | |
| }; | |
| const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newMonth = e.target.value; | |
| if (onChange) { | |
| onChange(`${year || ""}-${newMonth}-${day || ""}`); | |
| } | |
| }; | |
| const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newYear = e.target.value; | |
| if (onChange) { | |
| onChange(`${newYear}-${month || ""}-${day || ""}`); | |
| } | |
| }; | |
| const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newDay = e.target.value; | |
| if (Number(newDay) > 31 || Number(newDay) < 1) return; | |
| if (onChange) { | |
| onChange(`${year || ""}-${month || ""}-${newDay}`); | |
| } | |
| }; | |
| const handleMonthChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newMonth = e.target.value; | |
| if (Number(newMonth) > 12 || Number(newMonth) < 1) return; | |
| if (onChange) { | |
| onChange(`${year || ""}-${newMonth}-${day || ""}`); | |
| } | |
| }; | |
| const handleYearChange = (e: React.ChangeEvent<HTMLInputElement>) => { | |
| const newYear = e.target.value; | |
| if (Number(newYear) < 1900 || Number(newYear) > new Date().getFullYear()) return; | |
| if (onChange) { | |
| onChange(`${newYear}-${month || ""}-${day || ""}`); | |
| } | |
| }; |
src/components/ui/datefield.tsx
Outdated
| export default function DateField({ value = "", onChange }: DateFieldProps) { | ||
| const [year, month, day] = value.split("-"); | ||
| const { t } = useTranslation(); |
There was a problem hiding this comment.
Add validation for the date string format.
The value.split("-") operation assumes a valid date string format. Add validation to handle invalid or malformed input.
- const [year, month, day] = value.split("-");
+ const [year, month, day] = value?.match(/^\d{4}-\d{2}-\d{2}$/) ? value.split("-") : ["", "", ""];📝 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.
| export default function DateField({ value = "", onChange }: DateFieldProps) { | |
| const [year, month, day] = value.split("-"); | |
| const { t } = useTranslation(); | |
| export default function DateField({ value = "", onChange }: DateFieldProps) { | |
| const [year, month, day] = value?.match(/^\d{4}-\d{2}-\d{2}$/) ? value.split("-") : ["", "", ""]; | |
| const { t } = useTranslation(); |
src/components/ui/datefield.tsx
Outdated
| interface DateFieldProps { | ||
| value?: string; | ||
| onChange?: (value: string) => void; | ||
| } |
There was a problem hiding this comment.
can we stick with Date type instead of string in the prop's interface for value and onChange?
src/components/ui/datefield.tsx
Outdated
| placeholder="DD" | ||
| value={day} | ||
| onChange={handleDayChange} | ||
| data-cy="dob-day-input" |
There was a problem hiding this comment.
forgot to remove this?
| data-cy="dob-day-input" |
src/components/ui/datefield.tsx
Outdated
| <Input | ||
| type="number" | ||
| placeholder="DD" | ||
| value={day} | ||
| onChange={handleDayChange} | ||
| data-cy="dob-day-input" | ||
| /> |
There was a problem hiding this comment.
can we restrict the individual inputs to it's respective possible values?
day - min: 1, max: 31, similarly for month and year too respectively.
| <Input | |
| type="number" | |
| placeholder="DD" | |
| value={day} | |
| onChange={handleDayChange} | |
| data-cy="dob-day-input" | |
| /> | |
| <Input | |
| type="number" | |
| placeholder="DD" | |
| value={day} | |
| onChange={handleDayChange} | |
| min={0} | |
| max={31} | |
| data-cy="dob-day-input" | |
| /> |
src/components/ui/datefield.tsx
Outdated
| const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newDay = e.target.value; | ||
| if (onChange) { | ||
| onChange(`${year || ""}-${month || ""}-${newDay}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
the implementation of date input in existing PatientRegistration form had auto focus change once a valid value is inputted, can we preserve that logic in this too?
ref:
care_fe/src/components/Patient/PatientRegistration.tsx
Lines 570 to 580 in 23806f6
|
👋 Hi, @NikhilA8606, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/components/ui/date-field.tsx (3)
6-11: Add JSDoc documentation and improve prop validation.The
DateFieldPropsinterface would benefit from:
- JSDoc documentation describing the component's purpose and usage
- Validation for the
idprop which appears unused in the component+/** + * Props for the DateField component. + * @param date - The current date value + * @param onChange - Callback fired when the date changes + * @param disabled - Whether the input fields are disabled + * @param id - Unique identifier for the field group + */ interface DateFieldProps { date?: Date; onChange?: (date?: Date) => void; disabled: boolean; id: string; }
24-68: Refactor duplicate date handling logic.The three handlers share similar date creation logic. Consider extracting this into a shared utility function.
+const createValidDate = (year: string, month: string, day: string): Date | undefined => { + const y = parseInt(year, 10) || 1900; + const m = (parseInt(month, 10) || 1) - 1; + const d = parseInt(day, 10) || 1; + const date = new Date(y, m, d); + return !isNaN(date.getTime()) ? date : undefined; +}; const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { const newDay = e.target.value; if (newDay.length > 0 && onChange) { - const updatedDate = new Date( - parseInt(year || "1900", 10), - (parseInt(month || "01", 10) || 1) - 1, - parseInt(newDay, 10) || 1, - ); - onChange(updatedDate); + const updatedDate = createValidDate(year, month, newDay); + if (updatedDate) { + onChange(updatedDate); + } } // ... rest of the handler };
1-118: Consider splitting into controlled and uncontrolled variants.The current implementation only supports controlled usage. Consider:
- Adding an uncontrolled variant with internal state management
- Creating a custom hook for date validation logic
- Adding support for different date formats and locales
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)src/components/Patient/PatientRegistration.tsx(2 hunks)src/components/ui/date-field.tsx(1 hunks)src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/PublicAppointments/PatientRegistration.tsx
- src/components/Patient/PatientRegistration.tsx
src/components/ui/date-field.tsx
Outdated
| const dayValue = parseInt(newDay, 10); | ||
| if (newDay.length <= 2 && dayValue >= 1 && dayValue <= 31) { | ||
| document.getElementById("dob-month-input")?.focus(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace direct DOM manipulation with React refs.
Using document.getElementById for focus management is not a React-friendly approach. Use refs instead.
+import { useRef } from "react";
+
export default function DateField({
// ... existing props
}) {
+ const monthInputRef = useRef<HTMLInputElement>(null);
+ const yearInputRef = useRef<HTMLInputElement>(null);
// In handleDayChange
- document.getElementById("dob-month-input")?.focus();
+ monthInputRef.current?.focus();
// In handleMonthChange
- document.getElementById("dob-year-input")?.focus();
+ yearInputRef.current?.focus();
// In Input components
- id="dob-month-input"
+ ref={monthInputRef}
- id="dob-year-input"
+ ref={yearInputRef}Also applies to: 52-55
src/components/ui/date-field.tsx
Outdated
| const year = date ? date.getFullYear().toString() : ""; | ||
| const month = date ? (date.getMonth() + 1).toString().padStart(2, "0") : ""; | ||
| const day = date ? date.getDate().toString().padStart(2, "0") : ""; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add date validation to prevent invalid date handling.
The current date parsing doesn't validate the input Date object. Consider adding validation to handle invalid dates gracefully.
- const year = date ? date.getFullYear().toString() : "";
- const month = date ? (date.getMonth() + 1).toString().padStart(2, "0") : "";
- const day = date ? date.getDate().toString().padStart(2, "0") : "";
+ const year = date && !isNaN(date.getTime()) ? date.getFullYear().toString() : "";
+ const month = date && !isNaN(date.getTime()) ? (date.getMonth() + 1).toString().padStart(2, "0") : "";
+ const day = date && !isNaN(date.getTime()) ? date.getDate().toString().padStart(2, "0") : "";📝 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 year = date ? date.getFullYear().toString() : ""; | |
| const month = date ? (date.getMonth() + 1).toString().padStart(2, "0") : ""; | |
| const day = date ? date.getDate().toString().padStart(2, "0") : ""; | |
| const year = date && !isNaN(date.getTime()) ? date.getFullYear().toString() : ""; | |
| const month = date && !isNaN(date.getTime()) ? (date.getMonth() + 1).toString().padStart(2, "0") : ""; | |
| const day = date && !isNaN(date.getTime()) ? date.getDate().toString().padStart(2, "0") : ""; |
src/components/ui/date-field.tsx
Outdated
| return ( | ||
| <div className="flex items-center gap-2"> | ||
| <div className="flex flex-col gap-1"> | ||
| <Label>{t("day")}</Label> | ||
| <Input | ||
| type="number" | ||
| placeholder="DD" | ||
| value={day} | ||
| onChange={handleDayChange} | ||
| disabled={disabled} | ||
| min={1} | ||
| max={31} | ||
| id="dob-day-input" | ||
| className="w-[10rem]" | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="flex flex-col gap-1"> | ||
| <Label>{t("month")}</Label> | ||
| <Input | ||
| type="number" | ||
| placeholder="MM" | ||
| value={month} | ||
| onChange={handleMonthChange} | ||
| disabled={disabled} | ||
| min={1} | ||
| max={12} | ||
| id="dob-month-input" | ||
| className="w-[10rem]" | ||
| /> | ||
| </div> | ||
|
|
||
| <div className="flex flex-col gap-1"> | ||
| <Label>{t("year")}</Label> | ||
| <Input | ||
| type="number" | ||
| placeholder="YYYY" | ||
| value={year} | ||
| onChange={handleYearChange} | ||
| disabled={disabled} | ||
| min={1900} | ||
| max={2100} | ||
| id="dob-year-input" | ||
| className="w-[10rem]" | ||
| /> | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and error handling in the UI.
The date input fields need accessibility improvements and error state handling:
- Add aria-labels for screen readers
- Add error states for invalid inputs
- Consider making the width configurable via props
<Input
type="number"
placeholder="DD"
value={day}
onChange={handleDayChange}
disabled={disabled}
min={1}
max={31}
- id="dob-day-input"
+ aria-label={t("day")}
+ aria-invalid={dayError}
+ aria-describedby={dayError ? `${id}-day-error` : undefined}
className="w-[10rem]"
/>
+{dayError && (
+ <span id={`${id}-day-error`} className="text-red-500 text-sm">
+ {dayError}
+ </span>
+)}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/ui/date-field.tsx (3)
22-28: 🛠️ Refactor suggestionAdd date validation to prevent invalid date handling.
The initialization of state from the date prop needs validation to handle invalid dates gracefully.
const [day, setDay] = useState( - date ? date.getDate().toString().padStart(2, "0") : "", + date && !Number.isNaN(date.getTime()) ? date.getDate().toString().padStart(2, "0") : "", ); const [month, setMonth] = useState( - date ? (date.getMonth() + 1).toString().padStart(2, "0") : "", + date && !Number.isNaN(date.getTime()) ? (date.getMonth() + 1).toString().padStart(2, "0") : "", );
43-100: 🛠️ Refactor suggestionReplace direct DOM manipulation with React refs.
Using document.getElementById for focus management is not a React-friendly approach.
Optimize event handlers to reduce code duplication.
The three event handlers share similar logic patterns and could be refactored for better maintainability.
+ type FieldType = 'day' | 'month' | 'year'; + + const handleFieldChange = ( + fieldType: FieldType, + value: string, + setter: (value: string) => void, + validation: { min: number; max: number; length: number }, + nextField?: FieldType + ) => { + setter(value); + + if ( + value.length === validation.length && + parseInt(value, 10) >= validation.min && + parseInt(value, 10) <= validation.max + ) { + const fields = { day, month, year, [fieldType]: value }; + + if (isValidDate(fields.year, fields.month, fields.day) && onChange) { + const updatedDate = new Date( + parseInt(fields.year, 10), + parseInt(fields.month, 10) - 1, + parseInt(fields.day, 10) + ); + onChange(updatedDate); + } + + if (nextField && refs[nextField].current) { + refs[nextField].current?.focus(); + } + } + }; + - const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { - const newDay = e.target.value; - setDay(newDay); - // ... existing logic - }; + const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => + handleFieldChange('day', e.target.value, setDay, + { min: 1, max: 31, length: 2 }, 'month');
102-147: 🛠️ Refactor suggestionImprove accessibility and error handling in the UI.
The date input fields need accessibility improvements and error state handling.
🧹 Nitpick comments (2)
src/components/ui/date-field.tsx (2)
7-12: Add JSDoc documentation for better maintainability.Consider adding JSDoc documentation to describe the component's purpose and props:
+/** + * A form field component for date input with separate day, month, and year fields. + */ interface DateFieldProps { + /** The current date value */ date?: Date; + /** Callback fired when the date changes */ onChange?: (date?: Date) => void; + /** Function to determine if the date input should be disabled */ disabled?: (date: Date) => boolean; + /** Unique identifier for the field group */ id: string; }
1-149: Consider extracting date manipulation logic into a custom hook.The date manipulation and validation logic could be extracted into a custom hook (e.g.,
useDateField) to:
- Improve reusability across different date input components
- Separate concerns between UI and date logic
- Make the component easier to test
Would you like me to help create this custom hook?
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Patient/PatientRegistration.tsx(2 hunks)src/components/ui/date-field.tsx(1 hunks)src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/PublicAppointments/PatientRegistration.tsx
- src/components/Patient/PatientRegistration.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/ui/date-field.tsx
[error] 36-36: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/hooks/useFileManager.tsx (2)
296-315: Consider enhancing error message accessibility.The UI changes look good and provide better error feedback. However, we can improve accessibility by:
- Connecting the error message to the textarea using ARIA attributes
- Adding error announcement for screen readers
<Textarea name="editFileName" id="archive-file-reason" rows={6} required placeholder="Type the reason..." value={archiveReason} onChange={(e) => setArchiveReason(e.target.value)} + aria-invalid={!!archiveReasonError} + aria-describedby={archiveReasonError ? "archive-reason-error" : undefined} className={cn( archiveReasonError && "border-red-500 focus-visible:ring-red-500", )} /> {archiveReasonError && ( - <p className="text-sm text-red-500">{archiveReasonError}</p> + <p id="archive-reason-error" className="text-sm text-red-500" role="alert">{archiveReasonError}</p> )}
Line range hint
507-509: Enhance error handling in downloadFile function.The catch block could provide more specific error messages based on the error type, improving the user experience when different types of download failures occur.
- } catch (err) { - toast.error(t("file_download_failed")); + } catch (err) { + if (err instanceof TypeError && err.message === "Network response was not ok.") { + toast.error(t("file_download_failed_network")); + } else if (err instanceof TypeError && err.message.includes("Failed to fetch")) { + toast.error(t("file_download_failed_offline")); + } else { + toast.error(t("file_download_failed")); + console.error("File download error:", err); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Form/FormFields/DateFormField.tsx(0 hunks)src/hooks/useFileManager.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/Form/FormFields/DateFormField.tsx
🔇 Additional comments (1)
src/hooks/useFileManager.tsx (1)
6-12: LGTM: Clean import organization.The new imports are well-organized and align with modern React practices for UI component composition.
…k#10048/Upgrading-old-UI-components-with-new-ones
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/ui/date-field.tsx (3)
30-41:⚠️ Potential issueImprove date validation logic.
The current implementation has two issues:
- Uses unsafe
isNaNwhich performs type coercion.- Lacks validation for future dates.
Apply this diff to improve the date validation:
const isValidDate = (year: string, month: string, day: string): boolean => { const parsedYear = parseInt(year, 10); const parsedMonth = parseInt(month, 10) - 1; const parsedDay = parseInt(day, 10); const testDate = new Date(parsedYear, parsedMonth, parsedDay); return ( - !isNaN(testDate.getTime()) && + !Number.isNaN(testDate.getTime()) && + testDate <= new Date() && testDate.getFullYear() === parsedYear && testDate.getMonth() === parsedMonth && testDate.getDate() === parsedDay ); };🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
43-100: 🛠️ Refactor suggestionReplace direct DOM manipulation with React refs.
The current implementation uses direct DOM manipulation for focus management, which is not a React-friendly approach.
Apply this diff to improve the event handlers:
+import { useRef } from "react"; + export default function DateField({ // ... existing props }) { + const monthInputRef = useRef<HTMLInputElement>(null); + const yearInputRef = useRef<HTMLInputElement>(null); // In handleDayChange - document.getElementById(`${id}-month-input`)?.focus(); + monthInputRef.current?.focus(); // In handleMonthChange - document.getElementById(`${id}-year-input`)?.focus(); + yearInputRef.current?.focus(); // In Input components for month and year - id={`${id}-month-input`} + ref={monthInputRef} - id={`${id}-year-input`} + ref={yearInputRef}
102-147: 🛠️ Refactor suggestionImprove accessibility and error handling.
The current implementation lacks proper accessibility attributes and error states.
Apply this diff to improve accessibility and UX:
<Input type="number" placeholder="DD" value={day} onChange={handleDayChange} min={1} max={31} id={`${id}-day-input`} className="w-[10rem]" - disabled={disabled ? disabled(new Date()) : false} + disabled={disabled} + aria-label={t("day")} + aria-invalid={!!error} + aria-describedby={error ? `${id}-error` : undefined} /> +{error && ( + <span id={`${id}-error`} className="text-red-500 text-sm"> + {error} + </span> +)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/components/Patient/PatientRegistration.tsx(2 hunks)src/components/ui/date-field.tsx(1 hunks)src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/ui/date-field.tsx
[error] 36-36: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (2)
src/components/Patient/PatientRegistration.tsx (1)
517-525: LGTM! The DateField integration looks good.The implementation correctly handles the date conversion and maintains the form's validation requirements.
src/pages/PublicAppointments/PatientRegistration.tsx (1)
321-328: LGTM! The DateField integration looks good.The implementation correctly handles the date conversion and maintains the form's validation requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/ui/date-field.tsx (1)
54-54: 🛠️ Refactor suggestionReplace direct DOM manipulation with React refs.
Using
document.getElementByIdfor focus management is not a React-friendly approach.Also applies to: 76-76
🧹 Nitpick comments (3)
src/components/ui/date-field.tsx (3)
9-14: Enhance component accessibility with additional props.Consider adding accessibility-related props to improve screen reader support:
interface DateFieldProps { date?: Date; onChange?: (date?: Date) => void; disabled?: boolean; id: string; + "aria-label"?: string; + "aria-describedby"?: string; + error?: string; }
37-94: Reduce code duplication in event handlers.The three event handlers share similar logic. Consider extracting common functionality:
+ const createDateFromParts = ( + yearStr: string, + monthStr: string, + dayStr: string, + ): Date | undefined => { + if (isValidDate(yearStr, monthStr, dayStr)) { + return new Date( + parseInt(yearStr), + parseInt(monthStr) - 1, + parseInt(dayStr), + ); + } + }; + + const handleInputChange = ( + value: string, + setValue: (value: string) => void, + nextInputRef: React.RefObject<HTMLInputElement> | null, + validation: { min: number; max: number; length: number }, + position: "day" | "month" | "year", + ) => { + setValue(value); + + if ( + value.length === validation.length && + parseInt(value) >= validation.min && + parseInt(value) <= validation.max + ) { + const parts = { + day: position === "day" ? value : day, + month: position === "month" ? value : month, + year: position === "year" ? value : year, + }; + + const newDate = createDateFromParts( + parts.year, + parts.month, + parts.day, + ); + if (newDate && onChange) { + onChange(newDate); + } + + nextInputRef?.current?.focus(); + } + };
96-141: Enhance UI with error states and mobile responsiveness.Consider the following improvements:
- Add error states for invalid inputs
- Improve mobile responsiveness with responsive width classes
- <div className="flex items-center gap-2"> + <div className="flex flex-col sm:flex-row items-start sm:items-center gap-2"> <div className="flex flex-col gap-1"> <Label>{t("day")}</Label> <Input type="number" placeholder="DD" value={day} onChange={handleDayChange} min={1} max={31} id={`${id}-day-input`} - className="w-[10rem]" + className="w-full sm:w-[10rem]" disabled={disabled} + aria-invalid={error ? "true" : undefined} /> + {error && ( + <span className="text-destructive text-sm">{error}</span> + )} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(2 hunks)src/components/ui/date-field.tsx(1 hunks)src/hooks/useFileManager.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/hooks/useFileManager.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
public/locale/en.json (2)
1690-1690: LGTM! Clear and consistent translation.The translation follows the established conventions for action buttons in the application.
2015-2015: LGTM! Well-structured translation with proper markup.The translation effectively uses HTML markup for emphasis and template variables for dynamic content.
src/components/ui/date-field.tsx (2)
32-35: LGTM! Good use of dayjs for date validation.The implementation correctly uses dayjs with strict parsing mode for reliable date validation.
24-30: 🛠️ Refactor suggestionAdd date validation when initializing state.
The current implementation doesn't validate the input date before accessing its properties. Use dayjs for consistent validation:
const [day, setDay] = useState( - date ? date.getDate().toString().padStart(2, "0") : "", + date && dayjs(date).isValid() + ? dayjs(date).date().toString().padStart(2, "0") + : "", ); const [month, setMonth] = useState( - date ? (date.getMonth() + 1).toString().padStart(2, "0") : "", + date && dayjs(date).isValid() + ? (dayjs(date).month() + 1).toString().padStart(2, "0") + : "", ); - const [year, setYear] = useState(date ? date.getFullYear().toString() : ""); + const [year, setYear] = useState( + date && dayjs(date).isValid() ? dayjs(date).year().toString() : "", + );Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/ui/date-field.tsx (2)
9-14: Enhance the DateFieldProps interface for better validation and accessibility.Consider adding the following props to improve component flexibility and accessibility:
maxYear?: number- to customize the maximum allowed yearminYear?: number- to customize the minimum allowed year (currently hardcoded to 1900)error?: string- for displaying validation errorsaria-label?: string- for screen readersaria-describedby?: string- for error descriptionsinterface DateFieldProps { date?: Date; onChange?: (date?: Date) => void; disabled?: boolean; id: string; + maxYear?: number; + minYear?: number; + error?: string; + "aria-label"?: string; + "aria-describedby"?: string; }
96-143: Enhance UI accessibility and error handling.The UI needs improvements for accessibility and error feedback:
- Add aria attributes for screen readers
- Add error states
- Consider adding helper text for valid ranges
<div className="flex items-center gap-2"> <div className="flex flex-col gap-1"> - <Label>{t("day")}</Label> + <Label htmlFor={`${id}-day-input`}>{t("day")}</Label> <Input type="number" placeholder="DD" value={day} onChange={handleDayChange} min={1} max={31} id={`${id}-day-input`} className="w-[10rem]" disabled={disabled} + aria-label={t("day")} + aria-invalid={!!error} + aria-describedby={error ? `${id}-error` : undefined} /> + {error && ( + <span id={`${id}-error`} className="text-red-500 text-sm"> + {error} + </span> + )} + <span className="text-gray-500 text-xs"> + {t("valid_range", { min: 1, max: 31 })} + </span> </div> // Similar changes for month and year inputs </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/date-field.tsx(1 hunks)
🔇 Additional comments (2)
src/components/ui/date-field.tsx (2)
32-35: LGTM! Good use of dayjs for date validation.The implementation correctly uses dayjs with strict parsing mode, which is the recommended approach for reliable date validation.
37-94: 🛠️ Refactor suggestionRefactor event handlers for better maintainability and React practices.
Several improvements needed:
- Replace DOM manipulation with React refs
- Extract common date creation logic
- Use dayjs consistently for date operations
Here's how to improve the handlers:
+import { useRef } from "react"; + export default function DateField({ // ... props }) { + const monthInputRef = useRef<HTMLInputElement>(null); + const yearInputRef = useRef<HTMLInputElement>(null); + + const createDate = (newYear: string, newMonth: string, newDay: string) => { + if (isValidDate(newYear, newMonth, newDay) && onChange) { + const date = dayjs(`${newYear}-${newMonth}-${newDay}`, "YYYY-MM-DD", true); + onChange(date.toDate()); + } + }; const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { const newDay = e.target.value; setDay(newDay); - if ( - newDay.length === 2 && - parseInt(newDay) >= 1 && - parseInt(newDay) <= 31 - ) { - if (isValidDate(year, month, newDay) && onChange) { - const updatedDate = new Date( - parseInt(year), - parseInt(month) - 1, - parseInt(newDay), - ); - onChange(updatedDate); - } - document.getElementById(`${id}-month-input`)?.focus(); + if (newDay.length === 2 && +newDay >= 1 && +newDay <= 31) { + createDate(year, month, newDay); + monthInputRef.current?.focus(); } }; // Similar changes for handleMonthChange and handleYearChangeUpdate the input refs in JSX:
<Input // ... other props - id={`${id}-month-input`} + ref={monthInputRef} /> <Input // ... other props - id={`${id}-year-input`} + ref={yearInputRef} />Likely invalid or redundant comment.
src/components/ui/date-field.tsx
Outdated
| const [day, setDay] = useState( | ||
| date ? date.getDate().toString().padStart(2, "0") : "", | ||
| ); | ||
| const [month, setMonth] = useState( | ||
| date ? (date.getMonth() + 1).toString().padStart(2, "0") : "", | ||
| ); | ||
| const [year, setYear] = useState(date ? date.getFullYear().toString() : ""); |
There was a problem hiding this comment.
what happens when the parent component changes the date value? this wouldn't get updated right?
src/components/ui/date-field.tsx
Outdated
| const isValidDate = (year: string, month: string, day: string): boolean => { | ||
| const parsedDate = dayjs(`${year}-${month}-${day}`, "YYYY-MM-DD", true); | ||
| return parsedDate.isValid(); | ||
| }; |
There was a problem hiding this comment.
can we keep this function outside the component?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/ui/date-field.tsx (2)
44-101: 🛠️ Refactor suggestionReplace direct DOM manipulation with React refs.
Using
document.getElementByIdfor focus management is not a React-friendly approach.Extract common date construction logic.
The date construction logic is duplicated across all handlers. Consider extracting it into a shared function:
+const constructDate = (year: string, month: string, day: string): Date | undefined => { + if (isValidDate(year, month, day)) { + return new Date( + parseInt(year), + parseInt(month) - 1, + parseInt(day) + ); + } + return undefined; +}; const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => { const newDay = e.target.value; setDay(newDay); if (newDay.length === 2 && parseInt(newDay) >= 1 && parseInt(newDay) <= 31) { - if (isValidDate(year, month, newDay) && onChange) { - const updatedDate = new Date( - parseInt(year), - parseInt(month) - 1, - parseInt(newDay) - ); + const updatedDate = constructDate(year, month, newDay); + if (updatedDate && onChange) { onChange(updatedDate); } // ... focus management } };
103-149: 🛠️ Refactor suggestionImprove accessibility and error handling in the UI.
The date input fields need accessibility improvements and error state handling.
Additionally, consider adding visual feedback for invalid dates:
<Input type="number" placeholder="DD" value={day} onChange={handleDayChange} min={1} max={31} id={`${id}-day-input`} - className="w-[10rem]" + className={`w-[10rem] ${ + day && !isValidDate(year, month, day) ? "border-red-500" : "" + }`} disabled={disabled} />
🧹 Nitpick comments (2)
src/components/ui/date-field.tsx (2)
9-14: Enhance props interface with common form field properties.Consider extending the interface with common form field props for better reusability:
interface DateFieldProps { date?: Date; onChange?: (date?: Date) => void; disabled?: boolean; id: string; + required?: boolean; + error?: string; + "aria-label"?: string; + "aria-describedby"?: string; + placeholder?: { + day?: string; + month?: string; + year?: string; + }; }
16-19: Move utility function to a separate file.Consider moving
isValidDateto a dedicated date utilities file for better code organization and reusability. Also, consider renaming it to be more specific (e.g.,isValidDateString).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ui/date-field.tsx(1 hunks)
🔇 Additional comments (1)
src/components/ui/date-field.tsx (1)
32-42: 🛠️ Refactor suggestionUse dayjs for safer state initialization.
The current implementation directly accesses Date methods without validation. Use dayjs for consistent and safe date handling:
useEffect(() => { - if (date) { + if (date && dayjs(date).isValid()) { + const dateObj = dayjs(date); - setDay(date.getDate().toString().padStart(2, "0")); - setMonth((date.getMonth() + 1).toString().padStart(2, "0")); - setYear(date.getFullYear().toString()); + setDay(dateObj.format("DD")); + setMonth(dateObj.format("MM")); + setYear(dateObj.format("YYYY")); } else { setDay(""); setMonth(""); setYear(""); } }, [date]);Likely invalid or redundant comment.
rithviknishad
changed the title
date-field reusable component; Migrated old DateFormFields to new UI components
|
@NikhilA8606 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Replaced DateFormField with DatePicker ui component
DatePickercomponent for date of birth selection in patient registrationThese are the components to be replaced.
Merge Checklist
@rithviknishad
Summary by CodeRabbit
Release Notes
New Features
DateFieldcomponent for improved date input handling.Improvements
Localization
Refactor
DateFormFieldcomponent.