enhanced Date Time component #9379
Conversation
WalkthroughThe changes made to the Changes
Assessment against linked issues
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (1)
src/components/Common/DateTextInput.tsx (1)
Line range hint
1-201: Consider enhancing component accessibility and type safetyThe component could benefit from several improvements:
- Add proper ARIA labels for screen readers
- Leverage dayjs for comprehensive date validation
- Define proper TypeScript types for the component's state
Consider these enhancements:
// Define proper types type DateInputField = 'date' | 'month' | 'year' | 'hour' | 'minute'; type EditingText = Record<DateInputField, string>; // Add aria labels <input aria-label={`Enter ${key}`} role="spinbutton" aria-valuemin={key === "date" ? 1 : key === "month" ? 1 : 0} aria-valuemax={key === "date" ? 31 : key === "month" ? 12 : key === "year" ? 2999 : key === "hour" ? 23 : 59} // ... other props /> // Use dayjs for validation const isValidDate = (date: EditingText) => { return dayjs(`${date.year}-${date.month}-${date.date} ${date.hour}:${date.minute}`).isValid(); };
| @@ -116,7 +116,7 @@ export default function DateTextInput(props: { | |||
| date: `${value ? formatUnfocused(value.getDate(), 0) : ""}`, | |||
| month: `${value ? formatUnfocused(value.getMonth() + 1, 1) : ""}`, | |||
| year: `${value ? formatUnfocused(value.getFullYear(), 2, 4) : ""}`, | |||
| hour: `${value ? formatUnfocused(value.getHours(), 3) : ""}`, | |||
| hour: `${value ? formatUnfocused(value.getHours() % 12, 3) : ""}`, | |||
There was a problem hiding this comment.
Hour format implementation needs revision
The current implementation has several issues:
- It forces 12-hour format instead of respecting system preferences as intended
- The modulo operation incorrectly handles midnight (0) and noon (12), both becoming "0"
- Missing AM/PM indicator makes 12-hour format ambiguous
Consider this implementation:
-hour: `${value ? formatUnfocused(value.getHours() % 12, 3) : ""}`,
+const is24Hour = Intl.DateTimeFormat(undefined, { hour: 'numeric' }).resolvedOptions().hour12 === false;
+hour: `${value ? formatUnfocused(is24Hour ? value.getHours() : (value.getHours() % 12 || 12), 3) : ""}`,Also, consider adding an AM/PM indicator if using 12-hour format:
const period = value?.getHours() >= 12 ? 'PM' : 'AM';| value.endsWith("/") || | ||
| value.endsWith(" ") || | ||
| value.endsWith(":") || | ||
| value.length > (key === "year" ? 3 : 1) | ||
| ) { | ||
| goToInput(i + 1); | ||
| goToInput((i + 1) % 5); |
There was a problem hiding this comment.
Field navigation logic needs improvement
The current implementation has several issues:
- The modulo operation
(i + 1) % 5assumes all fields are present, but breaks whenallowTimeis false - Missing validation before transitioning could lead to invalid dates
- Automatic transition on delimiters might interfere with manual editing
Consider this implementation:
- value.endsWith("/") ||
- value.endsWith(" ") ||
- value.endsWith(":") ||
- value.length > (key === "year" ? 3 : 1)
- ) {
- goToInput((i + 1) % 5);
+ (value.endsWith("/") && ["date", "month"].includes(key)) ||
+ (value.endsWith(":") && key === "hour") ||
+ value.length > (key === "year" ? 3 : 1)
+ ) {
+ const maxFields = allowTime ? 5 : 3;
+ const nextIndex = (i + 1) % maxFields;
+ // Validate current field before moving
+ const currentValue = parseInt(value);
+ const isValid = !isNaN(currentValue) &&
+ currentValue <= (key === "date" ? 31 :
+ key === "month" ? 12 :
+ key === "year" ? 2999 :
+ key === "hour" ? 23 : 59);
+ if (isValid) {
+ goToInput(nextIndex);
+ }📝 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.
| value.endsWith("/") || | |
| value.endsWith(" ") || | |
| value.endsWith(":") || | |
| value.length > (key === "year" ? 3 : 1) | |
| ) { | |
| goToInput(i + 1); | |
| goToInput((i + 1) % 5); | |
| (value.endsWith("/") && ["date", "month"].includes(key)) || | |
| (value.endsWith(":") && key === "hour") || | |
| value.length > (key === "year" ? 3 : 1) | |
| ) { | |
| const maxFields = allowTime ? 5 : 3; | |
| const nextIndex = (i + 1) % maxFields; | |
| // Validate current field before moving | |
| const currentValue = parseInt(value); | |
| const isValid = !isNaN(currentValue) && | |
| currentValue <= (key === "date" ? 31 : | |
| key === "month" ? 12 : | |
| key === "year" ? 2999 : | |
| key === "hour" ? 23 : 59); | |
| if (isValid) { | |
| goToInput(nextIndex); | |
| } |
rithviknishad
added
needs testing
needs review
changes required
and removed
needs testing
needs review
labels
|
@ohcnetwork/care-fe-code-reviewers I had made commit with time taking hour as 12 for 12 am or pm, it failed 1 test, but when i reverted it back to 00 it passed all the test cases. I tried to run cypress tests locally, but, in all tests, page was loading till timeout Please guide me on how to proceed further |
rithviknishad
added
needs testing
needs review
and removed
question
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Common/DateInputV2.tsx (2)
69-69: Consider edge cases in hours calculationWhile the current implementation works for most cases, it might need special handling for midnight (0) and noon (12) in 12-hour format. In 12-hour format, both should display as "12" rather than "0".
Consider this alternative:
-const hours = is24Hour ? dayjs(value).hour() : dayjs(value).hour() % 12; +const hours = is24Hour ? dayjs(value).hour() : (dayjs(value).hour() % 12) || 12;
632-662: Consider extracting time input rendering logicWhile the implementation is functionally correct, the complex nested conditions and repeated logic could be simplified for better maintainability.
Consider extracting the time input rendering logic into a separate component:
interface TimeInputProps { option: number; value: number; name: string; is24Hour: boolean; onChange: (value: number) => void; } const TimeInput: React.FC<TimeInputProps> = ({ option, value, name, is24Hour, onChange }) => { const isSelected = name === "Hours" && option === 12 ? [0, 12].includes(value) : value === option; return ( <button type="button" className={`flex aspect-square w-9 shrink-0 items-center justify-center rounded-md border transition-all ${ isSelected ? "border-primary-600 bg-primary-500 font-bold text-white" : "border-gray-200 hover:bg-secondary-300" } text-sm`} onClick={() => onChange(option)} data-selected={isSelected} > {option.toLocaleString("en-US", { minimumIntegerDigits: 2, useGrouping: false, })} </button> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Common/DateInputV2.tsx(6 hunks)src/components/Common/DateTextInput.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Common/DateTextInput.tsx
🔇 Additional comments (4)
src/components/Common/DateInputV2.tsx (4)
157-160: LGTM: Improved time handling logic
The changes improve maintainability by:
- Removing dependency on previous AM/PM state
- Automatically determining AM/PM based on hour value in 24-hour mode
317-317: LGTM: Consistent date format usage
The implementation ensures consistency between the display format and internal representation by using the same dateFormat.
570-571: LGTM: Time options configuration
The implementation correctly handles hour ranges for both 24-hour (0-23) and 12-hour (1-12) formats, following standard time input patterns.
53-56: LGTM: Time format detection implementation
The implementation correctly uses the browser's Intl API to detect system time preferences, addressing the PR objective of aligning with user system time preferences.
Let's verify the time format detection works across different locales:
✅ Verification successful
Let me gather more information about the time format handling across the codebase to ensure there are no inconsistencies.
Time format detection implementation is consistent and correct
The implementation in DateInputV2.tsx correctly handles time format preferences and is consistent with the rest of the codebase. Key findings:
- The component properly detects system time preferences using
Intl.DateTimeFormat - The format strings (
HH:mmfor 24-hour andhh:mm afor 12-hour) are consistently used across components - The same detection approach is used in both
DateInputV2.tsxandDateTextInput.tsx - The codebase has centralized date/time format constants in
Utils/utils.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the component handles different locale time formats correctly
# Look for any hardcoded time format assumptions in the codebase
rg -g '*.{ts,tsx,js,jsx}' -A 3 'hour12|timeStyle|hourCycle'
Length of output: 1001
Script:
#!/bin/bash
# Check for any other date/time format related code and moment.js usage
rg -g '*.{ts,tsx,js,jsx}' -A 3 'DD/MM/YYYY|HH:mm|hh:mm a|moment\('
Length of output: 9620
|
@nihal467 @rithviknishad please review this |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Common/DateInputV2.tsx (1)
775-805: Consider extracting time input rendering logic.The time input rendering logic is complex and could benefit from being extracted into a separate component for better maintainability. This would also make it easier to test the time input functionality in isolation.
+ // TimeInput.tsx + interface TimeInputProps { + is24Hour: boolean; + value: number; + onChange: (value: number) => void; + options: number[]; + name: string; + } + + const TimeInput: React.FC<TimeInputProps> = ({ is24Hour, value, onChange, options, name }) => { + // Extract the time input rendering logic here + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/DateInputV2.tsx(6 hunks)
🔇 Additional comments (4)
src/components/Common/DateInputV2.tsx (4)
53-56: LGTM: Robust time format detection implementation.
The implementation correctly uses Intl.DateTimeFormat to detect the system's time format preference, which aligns with the PR objective of respecting user system preferences.
713-714: LGTM: Improved hour options handling.
The hour options are now correctly generated based on the time format:
- 24-hour format: 0-23
- 12-hour format: 1-12
800-803: LGTM: Consistent number formatting.
The use of toLocaleString with minimumIntegerDigits ensures consistent two-digit display of numbers, which improves readability and user experience.
69-69: Verify edge cases in hour calculations.
The hour calculations and AM/PM logic have been updated to handle both 12-hour and 24-hour formats. However, there are some edge cases to consider:
- When switching between formats, ensure the hour value is correctly preserved
- Verify behavior at midnight (00:00) and noon (12:00)
Also applies to: 167-170
✅ Verification successful
Let's search for the test files and any relevant test cases first. If we don't find specific tests, we can also look at the implementation to understand how these edge cases are handled.
If the above doesn't yield enough information:
Let's examine the time-related logic in the DateInputV2 component to understand how edge cases are handled. The previous search showed us the hours calculation, but we need to see more context around the AM/PM and format handling.
Edge cases in hour calculations are properly handled
After analyzing the implementation, the hour calculations and AM/PM logic are correctly implemented with proper handling of edge cases:
- Format switching: The code correctly handles 24-hour to 12-hour conversion using modulo operation (
hour() % 12), and the AM/PM is properly determined based on whether the hour is > 11 - Midnight/Noon handling:
- In 24-hour mode: hours are used directly from dayjs
- In 12-hour mode: hours are properly converted using modulo, and AM/PM is set based on the original 24-hour value
- Time changes are handled through
handleTimeChangewhich correctly manages hour conversions when switching AM/PM
The implementation uses dayjs's built-in hour handling functions and includes proper format detection using Intl.DateTimeFormat, making it robust for different locales and time formats.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering hour format transitions
rg -A 5 "test.*hour.*format|test.*time.*format"
Length of output: 49
Script:
#!/bin/bash
# Search for test files related to DateInputV2 component
fd "DateInputV2.*test" --type f
# Search for any test cases involving hour calculations or format changes
rg -A 10 "describe.*DateInputV2|test.*hour|test.*format" --type ts --type tsx
Length of output: 141
Script:
#!/bin/bash
# Look at the implementation of DateInputV2 to understand hour calculations
ast-grep --pattern 'const hours = $_'
# Search for any AM/PM or format-related logic
rg -A 5 "ampm|is24Hour|format" --type ts --type tsx
Length of output: 408
Script:
#!/bin/bash
# Let's try searching without file type restrictions
rg -A 10 "ampm|is24Hour|format" src/components/Common/DateInputV2.tsx
# Also look for any time-related logic in the same file
rg -A 5 "dayjs.*hour|setHour|meridiem" src/components/Common/DateInputV2.tsx
Length of output: 6239
| className={`flex aspect-square w-9 shrink-0 items-center justify-center rounded-md border transition-all ${(input.name === "Hours" && option === 12 ? [0, 12].includes(input.value) : input.value === option) ? "border-primary-600 bg-primary-500 font-bold text-white" : "border-gray-200 hover:bg-secondary-300"} text-sm`} | ||
| onClick={() => | ||
| input.onChange(option as any) | ||
| } | ||
| data-selected={ | ||
| (input.name === "Hours" && option === 12 | ||
| ? [0, 12].includes(input.value) | ||
| : input.value === option) && | ||
| j + 1 >= input.options.length && | ||
| j + 1 <= input.options.length * 2 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify complex conditional logic.
The data-selected attribute contains complex nested conditions that are hard to understand and maintain. Consider extracting this logic into a helper function for better readability.
+ const isOptionSelected = (option: number, value: number, name: string) => {
+ if (name === "Hours" && option === 12) {
+ return [0, 12].includes(value);
+ }
+ return value === option;
+ };
- data-selected={
- (input.name === "Hours" && option === 12
- ? [0, 12].includes(input.value)
- : input.value === option) &&
- j + 1 >= input.options.length &&
- j + 1 <= input.options.length * 2
- }
+ data-selected={
+ isOptionSelected(option, input.value, input.name) &&
+ j + 1 >= input.options.length &&
+ j + 1 <= input.options.length * 2
+ }Committable suggestion skipped: line range outside the PR's diff.
Proposed Changes
Closes Improve the Date and Time component in the CARE #8994
Change 1: The text area and selection component for date and time should align with the user’s system time preference.
Change 2: Users should be allowed to enter "01" in the minutes part of the component. Similarly, this behavior should be allowed for all other date and hour parts as well
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes