feat(core/date-input): add clear method to reset value and validation…#2543
Conversation
… state Co-authored-by: Copilot <copilot@github.com>
🦋 Changeset detectedLatest commit: 7db8833 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a clear() method to the ix-date-input component and refines its validation logic to better handle novalidate forms and programmatic value resets. The changes include a new utility for clearing inputs and extensive regression tests. Review feedback suggests that date parsing should still be performed even when visual validation is suppressed to avoid invalid internal states. Furthermore, the reviewer recommends explicitly synchronizing validation classes during programmatic clears and ensuring that asynchronous validation calls are correctly awaited in property watchers.
… validation Co-authored-by: Copilot <copilot@github.com>
…e-is-empty-for-date
Co-authored-by: Copilot <copilot@github.com>
…x dates Co-authored-by: Copilot <copilot@github.com>
…e-is-empty-for-date
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a clear() method to the ix-date-input component and refactors its validation logic to correctly handle novalidate forms and programmatic value resets. The changes include adding a syncValidationClasses method, updating the value watcher, and providing comprehensive regression tests. Feedback focuses on ensuring that asynchronous validation methods are properly awaited, stale validation classes are removed when validation is suppressed (e.g., in novalidate forms), and that accessibility coverage is maintained with an axe-based component test as per the repository style guide.
| @Watch('value') watchValuePropHandler(newValue: string) { | ||
| if (!newValue && this.required && !this.isClearing) { | ||
| this.touched = true; | ||
| this.syncValidationClasses(); | ||
| } | ||
| this.onInput(newValue); | ||
| } |
There was a problem hiding this comment.
The syncValidationClasses method is asynchronous and should be awaited to ensure the visual state is updated correctly before proceeding. Additionally, syncValidationClasses should be called regardless of whether the new value is empty to ensure that the ix-invalid--required class is removed when a value is provided to a previously empty required field.
| @Watch('value') watchValuePropHandler(newValue: string) { | |
| if (!newValue && this.required && !this.isClearing) { | |
| this.touched = true; | |
| this.syncValidationClasses(); | |
| } | |
| this.onInput(newValue); | |
| } | |
| @Watch('value') | |
| async watchValuePropHandler(newValue: string) { | |
| if (!newValue && this.required && !this.isClearing) { | |
| this.touched = true; | |
| } | |
| await this.syncValidationClasses(); | |
| await this.onInput(newValue); | |
| } |
| private async syncValidationClasses(): Promise<void> { | ||
| const skipValidation = await shouldSuppressInternalValidation(this); | ||
| if (skipValidation) { | ||
| return; | ||
| } | ||
|
|
||
| const hasValue = !!this.value; | ||
| if (this.required) { | ||
| this.hostElement.classList.toggle( | ||
| 'ix-invalid--required', | ||
| !hasValue && this.touched | ||
| ); | ||
| } else { | ||
| this.hostElement.classList.remove('ix-invalid--required'); | ||
| } | ||
| } |
There was a problem hiding this comment.
When skipValidation is true (e.g., in a novalidate form), the component should ensure that any existing validation classes are removed from the host element. The current implementation returns early, which might leave stale error indicators if the validation state changes dynamically.
private async syncValidationClasses(): Promise<void> {
const skipValidation = await shouldSuppressInternalValidation(this);
const hasValue = !!this.value;
const isInvalidRequired =
!skipValidation && !!this.required && !hasValue && this.touched;
this.hostElement.classList.toggle('ix-invalid--required', isInvalidRequired);
}
References
- Maintain consistency with existing component patterns (e.g., using a generic customError for validation) over introducing more specific but inconsistent implementations.
| }); | ||
| }); | ||
|
|
||
| regressionTest.describe('date-input validation scenarios', () => { |
There was a problem hiding this comment.
Per the repository style guide, component changes require accessibility coverage with an axe-based component test. Please add an automated accessibility check for the date-input component, especially covering the new validation states.
References
- For component changes, check that accessibility coverage exists with an axe-based component test. (link)
| const skipValidation = await shouldSuppressInternalValidation(comp); | ||
| if (skipValidation) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
If validation is suppressed via novalidate, any existing visual validation classes (like ix-invalid--validity-invalid) should be removed to ensure the UI correctly reflects the suppressed state.
| const skipValidation = await shouldSuppressInternalValidation(comp); | |
| if (skipValidation) { | |
| return; | |
| } | |
| const skipValidation = await shouldSuppressInternalValidation(comp); | |
| if (skipValidation) { | |
| comp.hostElement.classList.remove('ix-invalid--validity-invalid'); | |
| return; | |
| } |
References
- Maintain consistency with existing component patterns (e.g., using a generic customError for validation) over introducing more specific but inconsistent implementations.
…hen-value-is-empty-for-date
|
1 participant
💡 What is the current behavior?
ix-date-input(not required) has an invalid value and the user clears the field, the component continues to display as invalidix-date-inputis required and the user clears the field, the component should show as invalid but was not consistently doingJIRA Number: #2595
🆕 What is the new behavior?
requiredand non-required statesclear()method resets both the value and the touched/validation state cleanlyfocusInputIfKeyboardModeextracted to shared picker-input.util.ts for reuse across picker-based inputs>=/<=operators (resolves SonarQube lint issue)Note: The clear() method on ix-date-input is tagged with
@since 5.1.0. Please verify this matches the actual target release version and we need to update the JSDoc tag if needed before merging.🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support