Skip to content
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.

Sign up for GitHub

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

Upgrading old UI components with new ones #10049

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
+86 −102
Open

Upgrading old UI components with new ones #10049

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdc745e
Upgrading old UI components with new ones
Jan 19, 2025
b4a49ef
Add disabled feature in DatePicker
Jan 19, 2025
78c2c51
datefield component added
Jan 20, 2025
18776b1
Fix dateValue formatting in render
Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 5 additions & 83 deletions src/components/Patient/PatientRegistration.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import SectionNavigator from "@/CAREUI/misc/SectionNavigator";

import { Button } from "@/components/ui/button";
import { Checkbox } from "@/components/ui/checkbox";
import DateField from "@/components/ui/datefield";
import {
Form,
FormControl,
Expand Down Expand Up @@ -512,89 +513,10 @@ export default function PatientRegistration(
render={({ field }) => (
<FormItem>
<FormControl>
<div className="flex items-center gap-2">
<div className="flex flex-col gap-1">
<FormLabel required>{t("day")}</FormLabel>

<Input
type="number"
placeholder="DD"
{...field}
value={
form.watch("date_of_birth")?.split("-")[2]
}
onChange={(e) => {
form.setValue(
"date_of_birth",
`${form.watch("date_of_birth")?.split("-")[0]}-${form.watch("date_of_birth")?.split("-")[1]}-${e.target.value}`,
);
const day = parseInt(e.target.value);
if (
e.target.value.length === 2 &&
day >= 1 &&
day <= 31
) {
document
.getElementById("dob-month-input")
?.focus();
}
}}
data-cy="dob-day-input"
/>
</div>

<div className="flex flex-col gap-1">
<FormLabel required>{t("month")}</FormLabel>

<Input
type="number"
id="dob-month-input"
placeholder="MM"
{...field}
value={
form.watch("date_of_birth")?.split("-")[1]
}
onChange={(e) => {
form.setValue(
"date_of_birth",
`${form.watch("date_of_birth")?.split("-")[0]}-${e.target.value}-${form.watch("date_of_birth")?.split("-")[2]}`,
);
const month = parseInt(e.target.value);
if (
e.target.value.length === 2 &&
month >= 1 &&
month <= 12
) {
document
.getElementById("dob-year-input")
?.focus();
}
}}
data-cy="dob-month-input"
/>
</div>

<div className="flex flex-col gap-1">
<FormLabel required>{t("year")}</FormLabel>

<Input
type="number"
id="dob-year-input"
placeholder="YYYY"
{...field}
value={
form.watch("date_of_birth")?.split("-")[0]
}
onChange={(e) =>
form.setValue(
"date_of_birth",
`${e.target.value}-${form.watch("date_of_birth")?.split("-")[1]}-${form.watch("date_of_birth")?.split("-")[2]}`,
)
}
data-cy="dob-year-input"
/>
</div>
</div>
<DateField
value={field.value}
onChange={field.onChange}
/>
</FormControl>
<FormMessage />
</FormItem>
Expand Down
4 changes: 3 additions & 1 deletion src/components/ui/date-picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ import {
interface DatePickerProps {
date?: Date;
onChange?: (date?: Date) => void;
disabled?: (date: Date) => boolean;
}

export function DatePicker({ date, onChange }: DatePickerProps) {
export function DatePicker({ date, onChange, disabled }: DatePickerProps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Add JSDoc documentation for the DatePicker component and its props

The component lacks documentation for its props, particularly the disabled prop's behavior. Please add JSDoc documentation to explain that the disabled prop is a function that returns true to disable a specific date.

Example documentation:

/**
 * A date picker component that allows selecting a date from a calendar.
 */
interface DatePickerProps {
  /** The currently selected date */
  date?: Date;
  /** Callback fired when the date changes */
  onChange?: (date?: Date) => void;
  /** Function to determine if a date should be disabled. Return true to disable the date. */
  disabled?: (date: Date) => boolean;
}
🔗 Analysis chain

Verify disabled prop documentation.

While the implementation looks good, ensure that the component's documentation or comments describe the expected behavior of the disabled function (e.g., return true to disable a date).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there's any documentation for the DatePicker component
rg -B 5 -A 1 "DatePicker" --type ts --type tsx | grep -i "doc\|comment"

Length of output: 99

Script:

#!/bin/bash
# Search for DatePicker without file type filtering
rg -B 5 -A 1 "DatePicker"

# Use ast-grep to find interface/type definitions and any JSDoc
ast-grep --pattern 'interface DatePickerProps {
  $$$
}'

# Also search for any type definition
ast-grep --pattern 'type DatePickerProps = {
  $$$
}'

# Look for any JSDoc comments before DatePicker
ast-grep --pattern '/**
  $$$
*/
$_DatePicker'

Length of output: 15231

const [open, setOpen] = useState(false);

return (
Expand All @@ -44,6 +45,7 @@ export function DatePicker({ date, onChange }: DatePickerProps) {
setOpen(false);
}}
initialFocus
disabled={disabled}
/>
</PopoverContent>
</Popover>
Expand Down
72 changes: 72 additions & 0 deletions src/components/ui/datefield.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's follow similar file naming conventions on how other reusable components are named here.

date-field.tsx instead of datefield.tsx.

I'd go with date-text-input.tsx since that'd more confidently convey what the component exactly is.

Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { useTranslation } from "react-i18next";

import { Input } from "@/components/ui/input";
import { Label } from "@/components/ui/label";

interface DateFieldProps {
value?: string;
onChange?: (value: string) => void;
}
Comment on lines +6 to +9
Copy link
Member

@rithviknishad rithviknishad Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we stick with Date type instead of string in the prop's interface for value and onChange?

Comment on lines +6 to +9
Copy link
Member

@rithviknishad rithviknishad Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's also add disabled: boolean and id: string props too.

id is required for cypress tests to recognise and use these inputs.

Copy link
Contributor Author

@NikhilA8606 NikhilA8606 Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the need of disabled props is here?
@rithviknishad


export default function DateField({ value = "", onChange }: DateFieldProps) {
const [year, month, day] = value.split("-");
const { t } = useTranslation();
Comment on lines +11 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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();


const handleDayChange = (e: React.ChangeEvent<HTMLInputElement>) => {
const newDay = e.target.value;
if (onChange) {
onChange(`${year || ""}-${month || ""}-${newDay}`);
}
};
Comment on lines +15 to +20
Copy link
Member

@rithviknishad rithviknishad Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const month = parseInt(e.target.value);
if (
e.target.value.length === 2 &&
month >= 1 &&
month <= 12
) {
document
.getElementById("dob-year-input")
?.focus();
}
}}


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 || ""}`);
}
};
Comment on lines +15 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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 || ""}`);
}
};


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}
data-cy="dob-day-input"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to remove this?

Suggested change
data-cy="dob-day-input"

/>
Comment on lines +40 to +46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

Comment on lines +40 to +46
Copy link
Member

@rithviknishad rithviknishad Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we restrict the individual inputs to it's respective possible values?

day - min: 1, max: 31, similarly for month and year too respectively.

Suggested change
<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"
/>

</div>

<div className="flex flex-col gap-1">
<Label>{t("month")}</Label>
<Input
type="number"
placeholder="MM"
value={month}
onChange={handleMonthChange}
data-cy="dob-month-input"
/>
</div>

<div className="flex flex-col gap-1">
<Label>{t("year")}</Label>
<Input
type="number"
placeholder="YYYY"
value={year}
onChange={handleYearChange}
data-cy="dob-year-input"
/>
</div>
</div>
);
}
26 changes: 8 additions & 18 deletions src/pages/PublicAppointments/PatientRegistration.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { toast } from "sonner";
import { z } from "zod";

import { Button } from "@/components/ui/button";
import { DatePicker } from "@/components/ui/date-picker";
import {
Form,
FormControl,
Expand All @@ -21,8 +22,6 @@ import { Label } from "@/components/ui/label";
import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group";
import { Textarea } from "@/components/ui/textarea";

import DateFormField from "@/components/Form/FormFields/DateFormField";

import { usePatientContext } from "@/hooks/usePatientUser";

import { GENDER_TYPES } from "@/common/constants";
Expand Down Expand Up @@ -320,24 +319,15 @@ export function PatientRegistration(props: PatientRegistrationProps) {
<FormItem className="flex flex-col">
<FormLabel required>{t("date_of_birth")}</FormLabel>
<FormControl>
<DateFormField
name="date_of_birth"
value={
<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)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add date validation and improve type safety.

The DatePicker implementation needs several improvements:

  1. Add validation to prevent future dates (previously handled by disableFuture).
  2. Add type validation for the date conversion.
  3. Sanitize the date input before form mutation.
  4. 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.

disabled={(date) =>
date <= new Date("1900-01-01") ||
date >= new Date()
}
/>
</FormControl>
<FormMessage />
Expand Down
Loading