feat(core/time-input): time input validation improvements#2629
feat(core/time-input): time input validation improvements#2629RamVinayMandal wants to merge 10 commits into
Conversation
- Added support for custom error messages for required fields via `i18nErrorRequired` property. - Implemented `reportValidity` method to trigger immediate visual validation. - Introduced `clear` method to reset input value and validation state. - Enhanced internal validation logic to handle required fields and invalid inputs more effectively. - Updated tests to cover new validation scenarios and ensure accessibility compliance. - Improved handling of focus events to manage dropdown visibility and input focus in keyboard mode.
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 522c06c The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 enhances the validation behavior of the ix-time-input component by adding clear() and reportValidity() methods, introducing the i18nErrorRequired property, and adding comprehensive Playwright component tests. Feedback on the changes suggests refining generic validation utilities (reportFieldValidity and syncCustomInputValidity) to explicitly check for null, undefined, or empty strings instead of using simple falsy checks, which could cause issues with valid falsy values. Additionally, it is recommended to remove a redundant lowercase readonly attribute from the input element and to eliminate hardcoded page.waitForTimeout delays in the tests in favor of Playwright's built-in auto-retrying assertions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ub.com:RamVinayMandal/ix into fix-ix-3322-time-input-validation-improvements
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/angular/src/components.ts`:
- Around line 2660-2668: This Angular proxy is auto-generated, so do not edit
the `inputs` or `methods` metadata in `components.ts` directly. Make the
underlying change in the Stencil output-target generation source that produces
the `ix-time-input` proxy, then regenerate the Angular wrappers so the
`inputs`/`methods` lists stay in sync with the generator.
In `@packages/core/src/components/input/input.util.ts`:
- Around line 387-390: The cleanup order in the input reset flow is wrong
because additionalCleanup runs while comp.value still holds the old selection.
Update the reset logic in input.util so comp.value is assigned to emptyValue
before invoking options?.additionalCleanup?.(), and keep
comp.updateFormInternalValue?.(emptyValue) aligned with that cleared state. This
will ensure callers like time-input’s syncPickerTimeFromValue() observe the
cleared value instead of the stale pre-clear one.
In `@packages/core/src/components/time-input/test/time-input.ct.ts`:
- Around line 924-960: The current regression test only verifies the default
required message, so add an additional coverage path in the time input test
suite that sets the public i18nErrorRequired API on ix-time-input and asserts
the custom required text appears after emptying the field. Reuse the existing
reportValidity() and invalid-state flow in time-input.ct.ts, and verify both the
visible ix-typography message and the required invalid class behavior for the
custom message case.
- Around line 12-16: The expectNoVisualValidation helper only checks CSS
classes, so it can miss a visible validation message. Update
expectNoVisualValidation in time-input.ct.ts to also assert the error
copy/validation text stays hidden, alongside the existing class checks. Use the
shared time input locators/assertions already present in the time-input tests so
all callers of expectNoVisualValidation cover the full “no visual validation”
contract.
In `@packages/core/src/components/time-input/time-input.tsx`:
- Around line 464-474: The reportValidity() JSDoc in time-input.tsx has a broken
sentence because a blank line splits “independently” from “of user interaction,”
causing TypeDoc to render it as separate paragraphs. Update the JSDoc above
reportValidity() so the sentence stays in one paragraph, keeping the description
continuous while preserving the rest of the API documentation text.
- Around line 406-418: The auto-default-to-now logic in componentWillLoad is
overriding the required-field behavior and also causing an early valueChange
emission via onInput. Update the TimeInput component so the
DateTime.now().toFormat(...) fallback in componentWillLoad is skipped when the
field is required (or otherwise intended to start empty), and avoid emitting
valueChange for this initial auto-seeded value. Review the componentWillLoad,
onInput, and syncValidityToFormInternals flow in time-input.tsx to keep required
inputs empty on first load while preserving non-required defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d562d51e-4f2a-42a4-8f77-7026d90af1d6
⛔ Files ignored due to path filters (3)
packages/angular/standalone/src/components.tsis excluded by!packages/angular/standalone/src/components.tspackages/react/src/components/components.server.tsis excluded by!packages/react/src/components/**packages/vue/src/components/ix-time-input.tsis excluded by!packages/vue/src/components/**
📒 Files selected for processing (8)
.changeset/feat-time-input-validation.mdpackages/angular/src/components.tspackages/core/src/components.d.tspackages/core/src/components/input/input.util.tspackages/core/src/components/time-input/test/time-input.ct.tspackages/core/src/components/time-input/time-input.tsxpackages/core/src/components/utils/input/picker-input.util.tspackages/core/src/components/utils/input/validation.ts
|
1 participant
💡 What is the current behavior?
ix-time-inputshows as invalid when cleared, even though empty should be validix-time-inputdoesn't consistently show as invalid when the value is removednovalidateforms still display visual validation feedback despite opting out of validationrequiredattributeJIRA: IX-2595 IX-3322
🆕 What is the new behavior?
Validation Fixes
reportValidity()novalidateforms: Suppress all visual validation feedback;reportValidity()overrides this suppressionrequiredattribute: Validation state updates immediately when togglingNew APIs
clear()method to clear value and validation state (removes touched flag and error classes)reportValidity()method to trigger validation and show errors immediatelyi18nErrorRequiredprop to customize the required-field error messageValidation Behavior
novalidatesuppression is overridden byreportValidity()to show errorsAdditional Improvements
Note: The
clear(),reportValidity()methods andi18nErrorRequiredprop on ix-time-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
Summary by CodeRabbit
New Features
Bug Fixes