Disallowing out of date range #9201
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (5)
src/components/Common/DateInputV2.tsx (5)
99-101: Ensure decade navigation respects minimum year constraintIn the "year" view, the condition may prevent users from navigating to the decade containing the minimum year. Users should be able to view and select the minimum year when applicable.
Apply this diff to adjust the logic:
if ( - !min || year.getFullYear() - 10 >= min.getFullYear() + !min || year.getFullYear() - 10 >= min.getFullYear() || + (year.getFullYear() > min.getFullYear() && year.getFullYear() - 10 < min.getFullYear()) ) { setYear((prev) => dayjs(prev).subtract(10, "year").toDate()); }
255-268: Provide user-friendly feedback when month selection is out of rangeThe error message "Cannot select month out of range" might not provide sufficient guidance to the user.
Consider customizing the error message to include the permissible range or suggest valid options.
272-285: Improve error message clarity for year selectionSimilar to the month selection, enhancing the error message for out-of-range year selection can improve user experience.
Provide more detailed feedback to the user, possibly indicating the valid year range.
639-644: Simplify nested ternary operators for better readabilityThe nested ternary operators in the
classNameprop reduce code clarity.Refactor the class assignment using conditional statements:
let className = "w-1/4 rounded-lg px-2 py-4 text-center text-sm font-semibold"; if (isSelectedMonth(i)) { className += " bg-primary-500 text-white cursor-pointer"; } else if (isMonthWithinConstraints(i)) { className += " text-secondary-700 hover:bg-secondary-300 cursor-pointer"; } else { className += " !text-secondary-400 !cursor-not-allowed"; }
664-675: Improve readability of year selection class assignmentsThe class assignment for the year selection also uses nested ternary operators that can be simplified.
Apply a similar refactoring as suggested for the month selection to enhance code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/DateInputV2.tsx(7 hunks)
🔇 Additional comments (3)
src/components/Common/DateInputV2.tsx (3)
220-233: Verify edge cases in isMonthWithinConstraints function
The isMonthWithinConstraints function appears to correctly validate months against the min and max constraints. Ensure that it handles edge cases where the month is exactly at the boundary of min or max.
Consider adding unit tests to cover boundary cases, such as selecting the month equal to min.getMonth() or max.getMonth().
235-246: Validate logic in isYearWithinConstraints function
The isYearWithinConstraints function seems to correctly enforce the year constraints. Ensure that it accurately handles years at the boundaries defined by min and max.
Add unit tests to confirm that selecting the years exactly equal to min.getFullYear() and max.getFullYear() is handled as expected.
121-123:
Correct year increment logic to respect maximum year
The current condition may allow users to navigate beyond the maximum year. The comparison should ensure that the new year does not exceed the maximum year.
Apply this diff to fix the logic:
if (
- !max || year.getFullYear() + 10 >= max.getFullYear()
+ !max || year.getFullYear() + 10 <= max.getFullYear()
) {
setYear((prev) => dayjs(prev).add(10, "year").toDate());
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/DateInputV2.tsx(7 hunks)
🔇 Additional comments (1)
src/components/Common/DateInputV2.tsx (1)
637-642: LGTM: Clear visual feedback for constrained options
The implementation effectively communicates date constraints to users through appropriate styling of disabled options.
Also applies to: 668-673
|
@rithviknishad @nihal467 can you please review it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Common/DateInputV2.tsx (1)
223-248: LGTM! Consider adding JSDoc comments.The constraint validation functions are well-implemented with proper edge case handling. Consider adding JSDoc comments to document the parameters and return values.
+/** + * Checks if the given month is within the min/max date constraints + * @param month - The month to check (0-11) + * @returns boolean indicating if the month is within constraints + */ const isMonthWithinConstraints = (month: number) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/DateInputV2.tsx(7 hunks)
🔇 Additional comments (2)
src/components/Common/DateInputV2.tsx (2)
257-289: LGTM! Robust implementation with proper edge case handling.
The implementation correctly:
- Validates selections before updating
- Adjusts dates for month transitions (e.g., March 31 -> February 28/29)
- Provides user feedback for invalid selections
Line range hint 643-679: LGTM! Clean implementation of constraint-based UI states.
The implementation properly handles:
- Visual feedback for selected items
- Disabled states for out-of-range options
- Proper cursor styles for interactive states
|
@Jacobjeevan I have done the changes and also suggested changes from coderabbitai |
|
Something I noticed while testing, month selection to default to the min here (same happened for max as well). 
|
|
@Jacobjeevan Can you review the changes |
Do the same for year as well. Test around with different parameters for min and max and check for weird behavior before requesting for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Common/DateInputV2.tsx (2)
268-270: Consider extracting error messages to constantsFor better maintainability and consistency, consider extracting the error messages to constants.
+const ERROR_MESSAGES = { + MONTH_OUT_OF_RANGE: "Cannot select month out of range", + YEAR_OUT_OF_RANGE: "Cannot select year out of range", +} as const; Notification.Error({ - msg: outOfLimitsErrorMessage ?? "Cannot select month out of range", + msg: outOfLimitsErrorMessage ?? ERROR_MESSAGES.MONTH_OUT_OF_RANGE, }); Notification.Error({ - msg: outOfLimitsErrorMessage ?? "Cannot select year out of range", + msg: outOfLimitsErrorMessage ?? ERROR_MESSAGES.YEAR_OUT_OF_RANGE, });Also applies to: 294-296
437-443: Consider extracting button disable conditions to helper functionsThe disable conditions for navigation buttons are complex and repeated. Consider extracting them to helper functions for better readability and maintainability.
+const isDecrementDisabled = (type: DatePickerType) => { + switch (type) { + case "date": + return !isDateWithinConstraints( + getLastDay(), + datePickerHeaderDate.getMonth() - 1, + datePickerHeaderDate.getFullYear(), + ); + case "month": + return min && datePickerHeaderDate.getFullYear() <= min.getFullYear(); + case "year": + return min && year.getFullYear() - 1 < min.getFullYear(); + default: + return false; + } +}; +const isIncrementDisabled = (type: DatePickerType) => { + switch (type) { + case "date": + return !isDateWithinConstraints( + getLastDay(), + datePickerHeaderDate.getMonth(), + datePickerHeaderDate.getFullYear(), + ); + case "month": + return max && datePickerHeaderDate.getFullYear() >= max.getFullYear(); + case "year": + return max && year.getFullYear() + 1 > max.getFullYear(); + default: + return false; + } +}; -disabled={!isDateWithinConstraints(/*...*/)} +disabled={isDecrementDisabled("date")} -disabled={min && datePickerHeaderDate.getFullYear() <= min.getFullYear()} +disabled={isDecrementDisabled("month")} -disabled={min && year.getFullYear() - 10 < min.getFullYear()} +disabled={isDecrementDisabled("year")} -disabled={!isDateWithinConstraints(/*...*/)} +disabled={isIncrementDisabled("date")} -disabled={max && datePickerHeaderDate.getFullYear() >= max.getFullYear()} +disabled={isIncrementDisabled("month")} -disabled={max && year.getFullYear() + 10 > max.getFullYear()} +disabled={isIncrementDisabled("year")}Also applies to: 457-461, 476-479, 516-522, 536-540, 555-558
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/DateInputV2.tsx(7 hunks)
🔇 Additional comments (2)
src/components/Common/DateInputV2.tsx (2)
222-247: LGTM! Well-implemented validation functions
The validation functions are well-designed with thorough checks:
isMonthWithinConstraints: Properly validates both the first and last day of the monthisYearWithinConstraints: Correctly handles year boundaries
651-656: LGTM! Well-structured UI rendering logic
The UI rendering logic for month and year selection is well-implemented:
- Uses classNames utility effectively for conditional classes
- Properly handles selected, enabled, and disabled states
Also applies to: 682-687
|
Hey @Jacobjeevan , I have made changes to the min and max limits. The min and max limits for both year and month are now working fine. I have tested all scenarios, and no unusual behavior has been observed. It is ready for review. |
|
@Jacobjeevan can you check the PR |
|
LGTM |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit