fix(DateInput): standardize with native HTML date input #669
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes the DateInput component by replacing custom date/time pickers with native HTML5 date and datetime-local inputs, simplifying the codebase and reducing external dependencies.
Key changes:
- Replaced react-datepicker and react-day-picker with native HTML inputs
- Consolidated separate DateInput and TimeInput components into a single DateInput component supporting both
type="date"andtype="datetime-local" - Refactored the
toISODateOnlyutility function with improved validation logic
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/components/DateInput/index.tsx | Complete rewrite to use native HTML date/datetime-local inputs instead of react-day-picker, with support for both date-only and datetime inputs |
| client/src/components/TimeInput/index.tsx | Removed - functionality consolidated into DateInput with type="datetime-local" |
| client/src/components/TimeInput/TimeInput.css | Removed - no longer needed after TimeInput deletion |
| client/src/components/ControlledInputs/CtlTimeInput.tsx | Removed - replaced by CtlDateInput with type="datetime-local" |
| client/src/components/controlpanel/EventsManager/FeeWaiverModal.tsx | Updated to use datetime-local input instead of separate date and time inputs, removed CtlTimeInput import |
| client/src/components/controlpanel/EventsManager/EventSettingsModal.tsx | Updated to use datetime-local inputs for all date fields, removed CtlTimeInput import, fixed destructuring spacing |
| client/src/utils/misc.ts | Refactored toISODateOnly with improved date validation and clearer logic flow |
| client/package.json | Removed react-datepicker, react-day-picker, and @types/react-datepicker dependencies |
| client/package-lock.json | Updated lockfile to reflect removed dependencies |
Files not reviewed (1)
- client/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
client/src/components/controlpanel/EventsManager/FeeWaiverModal.tsx:160
- The value prop is redundant here since the Controller component already manages the field value and passes it to DateInput (see CtlDateInput.tsx line 35). The getValues call is unnecessary and the value prop can be removed without affecting functionality.
<CtlDateInput
type="datetime-local"
name="expirationDate"
control={control}
rules={required}
label="Expiration Date"
value={getValues("expirationDate")}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <input | ||
| {...props} | ||
| type={type} | ||
| placeholder={"mm/dd/yyyy"} |
There was a problem hiding this comment.
The placeholder is hardcoded to "mm/dd/yyyy" but this doesn't accurately represent the expected format for datetime-local inputs, which should include time (e.g., "mm/dd/yyyy, hh:mm am/pm"). Consider making the placeholder dynamic based on the type prop, or allowing it to be customized via props.
Comment on lines
+24
to
+118
| export default function DateInput({ | ||
| type = "date", | ||
| className, | ||
| label, | ||
| labelClassName, | ||
| inputClassName, | ||
| inlineLabel, | ||
| error, | ||
| required, | ||
| ...props | ||
| }: DateInputProps) { | ||
| const calculatedValue = () => { | ||
| if (!props.value) return ""; | ||
|
|
||
| const popperRef = useRef<HTMLInputElement>(null); | ||
| const [popperElement, setPopperElement] = useState<HTMLDivElement | null>( | ||
| null | ||
| ); | ||
| const popper = usePopper(popperRef.current, popperElement, { | ||
| placement: popupPlacement, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (value) { | ||
| const initValue = typeof value === "string" ? value : value.toISOString(); | ||
| const date = parseISO(initValue); | ||
| if (isValid(date)) { | ||
| formatAndSetInputValue(date); | ||
| setSelected(date); | ||
| } | ||
| if (type === "date") { | ||
| const date = | ||
| props.value instanceof Date ? props.value : new Date(props.value); | ||
| // Check if date is valid before calling toISOString() | ||
| if (isNaN(date.getTime())) return ""; | ||
| return date.toISOString().split("T")[0]; | ||
| } else if (type === "datetime-local") { | ||
| const dt = | ||
| props.value instanceof Date ? props.value : new Date(props.value); | ||
| // Check if date is valid before processing | ||
| if (isNaN(dt.getTime())) return ""; | ||
| const offset = dt.getTimezoneOffset(); | ||
| const localDt = new Date(dt.getTime() - offset * 60 * 1000); | ||
| return localDt.toISOString().slice(0, 16); | ||
| } | ||
| }, [value, setSelected, setInputValue]); | ||
|
|
||
| const handleInputChange: ChangeEventHandler<HTMLInputElement> = (e) => { | ||
| if (disabled) return; | ||
| const date = parse(e.currentTarget.value, "MM/dd/yyyy", new Date()); | ||
| if (isValid(date)) { | ||
| formatAndSetInputValue(date); | ||
| setSelected(date); | ||
| } else { | ||
| setSelected(undefined); | ||
| } | ||
| return ""; | ||
| }; | ||
|
|
||
| const handleOpenDialog = () => { | ||
| if (disabled) return; | ||
| setIsPopperOpen(true); | ||
| }; | ||
| const onChangeHandler = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (!props.onChange) return; | ||
|
|
||
| const handleDaySelect = (date: Date | undefined) => { | ||
| if (disabled) return; | ||
| if (!date) return; | ||
| setSelected(date); | ||
| formatAndSetInputValue(date); | ||
| onChange(date); | ||
| setIsPopperOpen(false); | ||
| }; | ||
|
|
||
| const formatAndSetInputValue = (date: Date | undefined) => { | ||
| if (!date) { | ||
| setInputValue(""); | ||
| // Handle empty input | ||
| if (!e.target.value) { | ||
| props.onChange(""); | ||
| return; | ||
| } | ||
| setInputValue(format(date, "MM/dd/yyyy")); | ||
|
|
||
| if (type === "date") { | ||
| const date = new Date(e.target.value); | ||
| // Validate date before calling toISODateOnly | ||
| if (isNaN(date.getTime())) { | ||
| props.onChange(""); | ||
| return; | ||
| } | ||
| props.onChange(toISODateOnly(date)); | ||
| } else if (type === "datetime-local") { | ||
| const dt = new Date(e.target.value); | ||
| // Validate date before passing it along | ||
| if (isNaN(dt.getTime())) { | ||
| props.onChange(""); | ||
| return; | ||
| } | ||
| props.onChange(dt); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <> | ||
| <div | ||
| ref={popperRef} | ||
| onClick={handleOpenDialog} | ||
| className={`conductor-date-input${ | ||
| inlineLabel ? " inline" : "" | ||
| } ${className} ${disabled ? "disabled-input" : ""}`} | ||
| > | ||
| {label !== null && ( | ||
| <label | ||
| className={`cdi-label${inlineLabel ? " inline" : ""}${ | ||
| required ? " form-required" : "" | ||
| }${error ? " form-error-label" : ""}`} | ||
| > | ||
| {label} | ||
| </label> | ||
| )} | ||
| <input | ||
| type="text" | ||
| placeholder={"mm/dd/yyyy"} | ||
| value={inputValue} | ||
| onChange={handleInputChange} | ||
| disabled={disabled} | ||
| /> | ||
| </div> | ||
| </> | ||
| <div | ||
| className={classNames( | ||
| "conductor-date-input", | ||
| inlineLabel ? "inline" : "", | ||
| className, | ||
| props.disabled ? "disabled-input" : "" | ||
| )} | ||
| > | ||
| {label !== null && ( | ||
| <label | ||
| className={classNames( | ||
| `cdi-label${inlineLabel ? " inline" : ""}`, | ||
| required ? " form-required" : "", | ||
| error ? " form-error-label" : "", | ||
| labelClassName | ||
| )} | ||
| > | ||
| {label} | ||
| {required ? "*" : ""} | ||
| </label> | ||
| )} | ||
| <input | ||
| {...props} | ||
| type={type} | ||
| placeholder={"mm/dd/yyyy"} | ||
| value={calculatedValue()} | ||
| onChange={onChangeHandler} | ||
| disabled={props.disabled} | ||
| className={classNames(inputClassName, error ? "form-error-input" : "")} | ||
| /> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default memo(DateInput); | ||
| } |
There was a problem hiding this comment.
The component is no longer wrapped with React.memo. The previous implementation used memo to prevent unnecessary re-renders when parent components update. Consider whether memoization is still needed for performance, especially since this component is used in forms that may re-render frequently.
| )} | ||
| > | ||
| {label} | ||
| {required ? "*" : ""} |
There was a problem hiding this comment.
The required asterisk is being added manually here, but the form-required class is also being applied to the label (line 98). This could result in duplicate asterisks if the CSS for form-required already adds one. Consider removing the manual asterisk on line 104 if the form-required class already handles this, or remove the class and keep only the manual asterisk for consistency.
Suggested change
| {required ? "*" : ""} |
Collaborator
|
🎉 This PR is included in version 2.114.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.