fix(date-input): modifying validation error containing date #3694
Conversation
🦋 Changeset detectedLatest commit: 7838f48 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@macci001 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update introduces a patch for the Changes
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (4)
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
Documentation and Community
|
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- .changeset/calm-kids-visit.md (1 hunks)
- packages/components/date-input/src/use-date-input.ts (1 hunks)
Additional comments not posted (4)
.changeset/calm-kids-visit.md (2)
1-3: LGTM!The changeset header is correctly formatted.
5-5: Fix the grammatical error.There is a minor grammatical error in the description. Apply this diff to fix it:
-The Date mentioned in the Error message of the date-picker is not according to the Locale. This PR adds an layer to the validationError to ensure correct formatting date in error messages. +The date mentioned in the error message of the date-picker is not according to the locale. This PR adds a layer to the validation error to ensure correct formatting of the date in error messages.packages/components/date-input/src/use-date-input.ts (2)
196-196: LGTM!The construction of the error messages is correct.
Also applies to: 211-211
198-198: LGTM!The insertion of the error messages into the
validationErrorsarray is correct.Also applies to: 213-213
macci001
force-pushed
the
macci001/modifying-validation-error-containing-date
branch
from
9ff2e80
to
fbd95e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- .changeset/calm-kids-visit.md (1 hunks)
- packages/components/date-input/src/use-date-input.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- .changeset/calm-kids-visit.md
Files skipped from review as they are similar to previous changes (1)
- packages/components/date-input/src/use-date-input.ts
macci001
changed the title
|
@coderabbitai review |
Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
packages/components/date-input/__tests__/date-input.test.tsx (2)
185-206: Approve the test case for minimum date value error messaging.The test case correctly sets up the locale and uses
parseDatefor date handling, ensuring that the error message is displayed according to the locale. However, consider adding a check to ensure that the error message is not only present but also correct in terms of its content.The test case is well-implemented and aligns with the PR objectives.
Consider enhancing the test by verifying the exact content of the error message, not just its presence.
208-229: Approve the test case for maximum date value error messaging.This test case is structured similarly to the minimum date value test case, ensuring consistency in testing. It correctly sets up the locale and uses
parseDatefor date handling. As with the previous test case, consider adding a check to ensure that the error message is not only present but also correct in terms of its content.The test case is well-implemented and aligns with the PR objectives.
Consider enhancing the test by verifying the exact content of the error message, not just its presence.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/components/date-input/tests/date-input.test.tsx (2 hunks)
- packages/components/date-input/src/use-date-input.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/components/date-input/src/use-date-input.ts
Additional comments not posted (1)
packages/components/date-input/__tests__/date-input.test.tsx (1)
4-10: Review the import statement forparseDate.The addition of the
parseDatefunction from@internationalized/dateis appropriate for the new functionality being tested. This function is used to ensure that dates are parsed correctly according to the locale settings, which is crucial for the new test cases.The import statement is correctly placed and necessary for the new tests.
|
Hello @wingkwong , |
|
will do |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
macci001
force-pushed
the
macci001/modifying-validation-error-containing-date
branch
from
49868a8
to
dd7aa1d
Compare
macci001
force-pushed
the
macci001/modifying-validation-error-containing-date
branch
from
dd7aa1d
to
fadddd6
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- .changeset/calm-kids-visit.md (1 hunks)
- packages/components/date-input/tests/date-input.test.tsx (2 hunks)
- packages/components/date-input/intl/messages.ts (1 hunks)
- packages/components/date-input/src/use-date-input.ts (3 hunks)
Additional comments not posted (7)
.changeset/calm-kids-visit.md (1)
1-5: Clear and concise changeset description.The changeset file accurately describes the patch update for the
@nextui-org/date-inputpackage, highlighting the focus on correcting date formatting in error messages according to the locale. This aligns well with the PR's objectives and provides clear information for version control history.packages/components/date-input/intl/messages.ts (1)
1-138: Well-structured localization messages.The file is well-organized, providing clear and consistent validation messages across multiple locales. The use of
{date}as a placeholder is appropriate and ensures that the date can be dynamically inserted based on the actual validation logic. This setup supports the PR's goal to handle locale-specific date formatting correctly.packages/components/date-input/__tests__/date-input.test.tsx (2)
4-10: Appropriate imports for testing.The imports from
@internationalized/dateand@react-aria/i18nare necessary for the new functionality being tested. These imports allow for the parsing of dates and setting of locale, which are crucial for the tests that check locale-specific error messaging.Also applies to: 13-13
185-229: Comprehensive tests for locale-based error messaging.The new test cases effectively validate that the
DateInputcomponent displays error messages correctly formatted according to the specified locale. These tests are crucial for ensuring that the component behaves as expected under different locale settings, aligning with the PR's objectives to enhance locale sensitivity in error messaging.packages/components/date-input/src/use-date-input.ts (3)
9-9: Approved: Import additions for localization.The addition of
useLocalizedStringFormatterfrom@react-aria/i18nis crucial for supporting localized string formatting, aligning with the PR's objectives to handle locale-specific date formats.
21-21: Approved: Import of internationalization messages.The import of
intlMessagesfrom../intl/messagesis necessary for utilizing localized messages in the validation logic, ensuring that error messages are consistent with the locale settings.
189-189: Approved: Use of localized string formatter.The instantiation of
useLocalizedStringFormatterwithintlMessagesis a good practice for generating locale-specific strings, which is essential for the functionality introduced in this PR.
Comments failed to post (2)
packages/components/date-input/src/use-date-input.ts (2)
210-229: Refactor suggestion: Simplify error handling logic for
maxValue.Similar to the
minValuehandling, the logic formaxValuevalidation errors is verbose and could benefit from refactoring to improve clarity and maintainability.Apply a similar refactor as suggested for
minValueerror handling:+function handleMaxValueError(validationDetails, maxValue, locale, state, stringFormatter) { + const indexInValidationErrors = Number(validationDetails.badInput) + + Number(validationDetails.customError) + + Number(validationDetails.patternMismatch) + + Number(validationDetails.rangeUnderflow); + const maxValueDate = new Date(maxValue.year, maxValue.month - 1, maxValue.day).toLocaleDateString(locale); + const timeZone = state.segments.filter((segment) => segment.type === "timeZoneName")[0]?.text ?? ""; + return stringFormatter.format("maxValidationMessage").replace("{date}", `${maxValueDate} ${timeZone}`); +} + - if (props.maxValue != undefined && validationDetails.rangeOverflow) { - const indexInValidationErrors: number = - Number(validationDetails.badInput) + - Number(validationDetails.customError) + - Number(validationDetails.patternMismatch) + - Number(validationDetails.rangeUnderflow); - const maxValueDate = new Date( - maxValue.year, - maxValue.month - 1, - maxValue.day, - ).toLocaleDateString(locale); - const timeZone = - state.segments.filter((segment) => segment.type === "timeZoneName")[0]?.text ?? ""; - const rangeOverflow = stringFormatter - .format("maxValidationMessage") - .replace("{date}", `${maxValueDate} ${timeZone}`); - validationErrors.splice(indexInValidationErrors, 1, rangeOverflow); - } + if (props.maxValue != undefined && validationDetails.rangeOverflow) { + const rangeOverflow = handleMaxValueError(validationDetails, maxValue, locale, state, stringFormatter); + validationErrors.splice(indexInValidationErrors, 1, rangeOverflow); + }Committable suggestion was skipped due to low confidence.
191-208: Refactor suggestion: Simplify error handling logic.
The logic for handling
minValuevalidation errors is complex and could be simplified. Consider using helper functions to reduce complexity and improve readability.Consider refactoring the error handling logic into a separate function to improve maintainability. Here's a suggested refactor:
+function handleMinValueError(validationDetails, minValue, locale, state, stringFormatter) { + const indexInValidationErrors = Number(validationDetails.badInput) + + Number(validationDetails.customError) + + Number(validationDetails.patternMismatch); + const minValueDate = new Date(minValue.year, minValue.month - 1, minValue.day).toLocaleDateString(locale); + const timeZone = state.segments.filter((segment) => segment.type === "timeZoneName")[0]?.text ?? ""; + return stringFormatter.format("minValidationMessage").replace("{date}", `${minValueDate} ${timeZone}`); +} + - if (props.minValue != undefined && validationDetails.rangeUnderflow) { - const indexInValidationErrors: number = - Number(validationDetails.badInput) + - Number(validationDetails.customError) + - Number(validationDetails.patternMismatch); - const minValueDate = new Date( - minValue.year, - minValue.month - 1, - minValue.day, - ).toLocaleDateString(locale); - const timeZone = - state.segments.filter((segment) => segment.type === "timeZoneName")[0]?.text ?? ""; - const rangeUnderflow = stringFormatter - .format("minValidationMessage") - .replace("{date}", `${minValueDate} ${timeZone}`); - validationErrors.splice(indexInValidationErrors, 1, rangeUnderflow); - } + if (props.minValue != undefined && validationDetails.rangeUnderflow) { + const rangeUnderflow = handleMinValueError(validationDetails, minValue, locale, state, stringFormatter); + validationErrors.splice(indexInValidationErrors, 1, rangeUnderflow); + }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/components/date-input/intl/messages.ts (1 hunks)
- packages/components/date-input/src/use-date-input.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- packages/components/date-input/intl/messages.ts
Files skipped from review as they are similar to previous changes (1)
- packages/components/date-input/src/use-date-input.ts
|
Applied the suggestions in: fadddd6 |
|
@macci001 can you explain a bit why we need to include |
|
Browser is responsible for the content of the validation messages and RA has no role to play in it. So in-order to cutomize error message based on the locale provided, we need to handle it by our self. |
|
Got it. Lemme take a look later |
| @@ -184,6 +186,48 @@ export function useDateInput<T extends DateValue>(originalProps: UseDateInputPro | |||
| isInvalid: ariaIsInvalid, | |||
| } = useAriaDateField({...originalProps, label, validationBehavior, inputRef}, state, domRef); | |||
|
|
|||
| const stringFormatter = useLocalizedStringFormatter(intlMessages) as any; | |||
There was a problem hiding this comment.
i think as any is not required.
| const indexInValidationErrors: number = | ||
| Number(validationDetails.badInput) + | ||
| Number(validationDetails.customError) + | ||
| Number(validationDetails.patternMismatch); |
There was a problem hiding this comment.
do we need to cater rangeUnderflow as well?
There was a problem hiding this comment.
I don't think we need rangeUnderflow here as only badInput, customError and patternMatch errors appears before rangeUnderflow error in validationErrors array. Hence, if some or all of them exist they will shift the index of rangeUnderflow in validationErrors.
But in case of rangeOverflow as we have badInput, customError , patternMatch and rangeUnderflow errors before it in validationErrors array, we need to consider all of them.
There was a problem hiding this comment.
May I know where did you get this info?
There was a problem hiding this comment.
So I assumed that the errors in the validationField will be populated according to the occurrence of the property in ValidityState.
But seems like I made a wrong assumption, as Errors related to Bad Input are populated in validationError after rangeUnderflow and rangeOverflow by RA .(ref).
Also, while scrapping through RA code, I understood that if ValidationError has rangeUnderflow/rangeOverflow then it can only have badInput error.
Now, as we might not consider the order of errors in ValidityState as order in ValidityError, it might be bit tricky to find out which error in ValidationError list corresponds to underflow/overflow.
It is clear that if exists in ValidationError, the rangeOverflow and rangeUnderflow will be its first element. So should we edit the first value in the list?
The reason why the rangeOverflow and rangeUnderflow will always be first element:
- ValidationErrors can either be obtained from controlledError, serverError, clientError, builtinValidation, currentValidity (ref)
- controlledError, serverError, clientError and currentValidity => All of these does not have true for the ValidityState corresponding to
rangeUnderflow,rangeOverflowandBadInput. rangeOverflow/rangeUnderflowhappens to be in the validation errors list if it is obtained from builtinValidation.- The order in which the errors are populated for builtinValidation is this.
- Hence, the
rangeOverflow/rangeUnderflowappears to be the first element if they do exist.
There was a problem hiding this comment.
Updated the following changes in 7838f48
- Added support for badInput message with i18n.
- Modifying the validationError according exactly how they were populated by RA
macci001
force-pushed
the
macci001/modifying-validation-error-containing-date
branch
from
33ae285
to
7838f48
Compare
There was a problem hiding this comment.
I think it's hard to manage error messages by country in NextUI. Validating and maintaining each language can be costly. Since React Aria is talking to the globalization team, it might be best to wait for their updates.
In the meantime, you could use React Aria hooks to show custom error messages.
Thanks for the PR!
Closes #3693
📝 Description
The PR fixes the date mentioned in the validation error messages of date-picker.
⛳️ Current behavior (updates)
Currently the date in the validation message of date-picker in based on the browser and not on the locale mentioned.
The reason behind this is from
react-ariaas it uses the browser for the validation.Below shows the current behaviour:
hideTimeZoneas true)hideTimeZoneas true)🚀 New behavior
Below shows the new behaviour:
hideTimeZoneas true)hideTimeZoneas true)💣 Is this a breaking change (Yes/No): No
Summary by CodeRabbit
New Features
Bug Fixes
Tests