feat(date-picker): add automatic validation for required fields#3814
feat(date-picker): add automatic validation for required fields#3814adrianschmidt-bot wants to merge 2 commits intoLundalogik:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWraps date-picker render in a Host, adds private hasInteracted state and isFieldInvalid() helper to delay required validation until after user interaction, updates native/calendar blur and change handlers to mark interaction, syncs MDC/Flatpickr validation, and adds a required-example component. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant DP as DatePicker
participant FP as Flatpickr
participant MDC as MDCTextField
Note over User,DP: User interacts with the picker
User->>DP: focus / open / input / click clear / blur
DP->>DP: set hasInteracted = true
alt non-native calendar path
DP->>FP: open calendar / request selection
FP-->>DP: selection / change
DP->>DP: update value, set hasInteracted
end
DP->>MDC: compute & apply isFieldInvalid()
DP-->>User: render input (clear icon shown only if value present && !required)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
adrianschmidt
force-pushed
the
feat/date-picker-required-validation
branch
from
0960f55 to
ae53964
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/date-picker/date-picker.tsx (3)
197-206:⚠️ Potential issue | 🟠 MajorRequired fields should not be clearable per Issue
#3646.The PR objective states this fixes Issue
#3646, which requires that "date pickers with required attribute should not be clearable." However, the current implementation still shows the clear icon (line 202) and allows clearing viaclearValue()whenrequiredis true.To align with the linked issue, the clear icon should be hidden and clearing should be prevented when
required && value:🔧 Proposed fix
const inputProps: any = { onAction: this.clearValue, }; - if (this.value && !this.readonly) { + if (this.value && !this.readonly && !this.required) { inputProps.trailingIcon = 'clear_symbol'; }
230-263: 🛠️ Refactor suggestion | 🟠 MajorWrap multiple JSX elements in
<Host>instead of an array literal.The render method returns an array
[...]containing multiple JSX elements. As per coding guidelines, when returning multiple JSX elements from therendermethod, use the special<Host>element instead.♻️ Proposed refactor
- return [ + return ( + <Host> <limel-input-field disabled={this.disabled} readonly={this.readonly} invalid={isInvalid} ... />, <limel-portal ... > ... - </limel-portal>, - ]; + </limel-portal> + </Host> + );Note: You'll need to add
Hostto the import from@stencil/core.As per coding guidelines: "When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element."
208-222:⚠️ Potential issue | 🟠 MajorNative picker path does not track interactions for required validation.
The native input path (iOS/Android) uses
nativeChangeHandlerwhich does not sethasInteracted = true, so required validation will never trigger when the user dismisses the native date picker without selecting a value. The custom picker path tracks interaction via theonBlurhandler on the input field, but the native input lacks this handler.Set
hasInteracted = trueinnativeChangeHandlerto ensure consistent validation behavior across platforms, or add anonBlurhandler to the native input field.
🤖 Fix all issues with AI agents
In `@src/components/date-picker/date-picker.tsx`:
- Around line 205-206: Add a single helper method isInvalid() on the component
that returns a boolean and centralizes the validation: if (this.readonly) return
false; otherwise return this.invalid || (this.hasInteracted && this.required &&
!this.value); then replace the inline computations in render() (where isInvalid
is currently computed) and in fixFlatpickrFocusBug() with calls to
this.isInvalid() so both places use the same logic.
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3814/ |
adrianschmidt-bot
force-pushed
the
feat/date-picker-required-validation
branch
from
ae53964 to
0d60a9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/date-picker/date-picker.tsx (1)
229-262: 🛠️ Refactor suggestion | 🟠 MajorWrap multiple JSX elements in
<Host>instead of an array literal.The
render()method returns an array literal with multiple top-level JSX elements. As per coding guidelines, StencilJS components should wrap multiple elements in the special<Host>element instead.♻️ Proposed fix
- return [ + return ( + <Host> <limel-input-field disabled={this.disabled} readonly={this.readonly} ... />, + /> <limel-portal ... > ... - </limel-portal>, - ]; + </limel-portal> + </Host> + );Note: You'll need to import
Hostfrom@stencil/core.
🤖 Fix all issues with AI agents
In `@src/components/date-picker/date-picker.tsx`:
- Around line 397-400: The isFieldInvalid() method currently returns
this.invalid || (this.hasInteracted && this.required && !this.value); update it
to respect readonly fields so readonly controls are never marked invalid: either
return false immediately if this.readonly, or include a !this.readonly check in
the composite condition (e.g. this.hasInteracted && this.required && !this.value
&& !this.readonly); modify the isFieldInvalid() function accordingly to use the
this.readonly property (matching the pattern used in chip-set.tsx).
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
adrianschmidt-bot
force-pushed
the
feat/date-picker-required-validation
branch
from
0d60a9f to
34a0b55
Compare
adrianschmidt-bot
force-pushed
the
feat/date-picker-required-validation
branch
2 times, most recently
from
95eb803 to
6e679c5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/date-picker/date-picker.tsx`:
- Around line 202-204: The clear icon is being hidden when a field is required
due to the condition in the DatePicker component; update the logic in the
date-picker (the block that sets inputProps.trailingIcon) so it only checks for
this.value and !this.readonly and removes the !this.required check (i.e., keep
inputProps.trailingIcon = 'clear_symbol' when required) — locate the code that
references this.value, this.readonly, this.required and inputProps.trailingIcon
in the date-picker component and remove the !this.required clause to restore the
clear button for required fields.
74eec21 to
7403b9d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
adrianschmidt
force-pushed
the
feat/date-picker-required-validation
branch
from
a054768 to
4517a44
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/date-picker/date-picker.tsx (1)
208-221:⚠️ Potential issue | 🟡 MinorNative path: dismissing the picker without changing a value won't trigger required validation.
The native render path has no
onBlurhandler, sohasInteractedis only set when the user actually changes the value (line 283 innativeChangeHandler). If a user opens and dismisses the native date picker without selecting anything,hasInteractedstaysfalseand the required-but-empty state is not shown.Compare with the non-native path where
hideCalendar(line 324) setshasInteracted = trueon every close, including close-without-selection.If you want parity, add an
onBlurhandler to the native<limel-input-field>:Proposed fix
<limel-input-field disabled={this.disabled} readonly={this.readonly} invalid={isInvalid} label={this.label} helperText={this.helperText} required={this.required} value={this.formatValue(this.value)} type={this.nativeType} onChange={this.nativeChangeHandler} + onBlur={this.handleNativeBlur} />And a handler:
private handleNativeBlur = () => { this.hasInteracted = true; };
|
⚡ d7dcb9a - Added |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/date-picker/date-picker.tsx`:
- Around line 293-295: handleNativeBlur currently sets this.hasInteracted = true
while disconnectedCallback calls hideCalendar(), which also flips
this.hasInteracted; to avoid hideCalendar doing double duty either (A)
explicitly set this.hasInteracted = true inside disconnectedCallback and keep
hideCalendar strictly UI-only, or (B) extract a small cleanup method (e.g.,
cleanupOnDisconnect) that sets this.hasInteracted and performs teardown, and
remove state mutation from hideCalendar; update references to handleNativeBlur,
disconnectedCallback, hideCalendar, and hasInteracted accordingly.
- Around line 407-415: The isFieldInvalid method (used to compute validation
state) doesn't account for the disabled prop so a control that becomes disabled
after user interaction can still appear invalid; update the isFieldInvalid()
implementation to return false when this.disabled is true (in addition to the
existing readonly check) so that disabled fields are never marked invalid —
modify the method in date-picker.tsx (function isFieldInvalid) to short-circuit
when this.disabled is truthy, consistent with how readonly is handled and
mirroring the intent in chip-set.tsx if you choose to update that file too.
|
⚡ Addressed CodeRabbit feedback in b2ffb84:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/date-picker/date-picker.tsx (1)
407-409:⚠️ Potential issue | 🟡 Minor
clearValuedoes not sethasInteracted, so clearing a programmatically-set value on a required field won't trigger validation.If
valueis set by the parent (not via user interaction),hasInteractedremainsfalse. The clear icon is shown (line 204), and clicking it callsclearValue, which emitsnull— but never setshasInteracted = true. After clearing,isFieldInvalid()returnsfalsebecausehasInteractedis stillfalse, so the required field won't show as invalid even though the user explicitly interacted.🐛 Proposed fix
private clearValue() { + this.hasInteracted = true; this.change.emit(null); }
@adrianschmidt-bot Please have a look at this feedback from CodeRabbit. |
|
Good catch! ⚡ Fixed in 9e10bbe — |
adrianschmidt
force-pushed
the
feat/date-picker-required-validation
branch
from
9e10bbe to
c88e24f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/date-picker/date-picker.tsx (1)
335-348:⚠️ Potential issue | 🟡 MinorPre-existing:
removeEventListenerfor the capture-phaseblurlistener is missing{ capture: true }.Not introduced in this PR, but worth noting: the
addEventListeneron line 316 registers with{ capture: true }, yetremoveEventListeneron line 340 omits it. Per the DOM spec, a capture listener can only be removed by also specifyingcapture: true. This meanspreventBlurFromCalendarContaineris never actually removed, causing a leak on each open/close cycle.Proposed fix
document.removeEventListener( 'blur', - this.preventBlurFromCalendarContainer + this.preventBlurFromCalendarContainer, + { capture: true } );
|
Good catch on the missing Since this is pre-existing code unrelated to this PR's changes, I'll open a separate issue to track this fix and keep this PR focused on the validation feature. |
|
Opened #3833 to track the listener leak fix. |
|
@adrianschmidt-bot in this branch, please run |
adrianschmidt-bot
force-pushed
the
feat/date-picker-required-validation
branch
from
c88e24f to
7d1ac40
Compare
|
Done! Rebased onto upstream/main and force-pushed. There were two conflicts in
Both resolved and the branch is now up to date. ✅ |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/date-picker/date-picker.tsx (2)
367-377: 🧹 Nitpick | 🔵 TrivialPotential double
hideCalendar()call on outside click (pre-existing).When a user clicks outside both the input and calendar:
mousedownfires →documentClickListener→hasInteracted = true,hideCalendar()- Input
blurfires →handleCalendarBlur→hasInteracted = true(no-op),hideCalendar()againThe second
hideCalendar()call is mostly idempotent, butfixFlatpickrFocusBug()will construct a secondnew MDCTextField(...)instance for the same DOM node. This is pre-existing (previouslyonBlurwas bound tohideCalendardirectly), but worth noting since the new handler amplifies the path. A simple guard (e.g., early-return if!this.showPortal) insidehideCalendarwould prevent the redundant work.♻️ Optional: guard against redundant calls
private hideCalendar() { + if (!this.showPortal) { + return; + } + setTimeout(() => { this.showPortal = false; });
342-355:⚠️ Potential issue | 🟠 MajorRemove the
{ capture: true }flag fromaddEventListeneror add it to the correspondingremoveEventListenerto prevent listener leakage.The
addEventListenerat line 322 registers theblurlistener with{ capture: true }, but theremoveEventListenerat line 347 omits the capture flag. Per the DOM spec, these are distinct listeners—a capture-phase listener and a non-capture-phase listener. TheremoveEventListenercall is a no-op and leaves the blur listener registered, causing a memory leak. Either remove the capture flag from theaddEventListenercall or add{ capture: true }to theremoveEventListenercall to ensure the listener is properly removed.
|
Thanks for the catch! ⚡ Fixed both issues in
|
adrianschmidt
force-pushed
the
feat/date-picker-required-validation
branch
from
d6baced to
405e091
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/date-picker/date-picker.tsx`:
- Around line 296-303: handleCalendarBlur currently sets hasInteracted = true
and calls hideCalendar(), which couples interaction tracking with UI dismissal;
make this intent explicit by adding a brief inline comment inside
handleCalendarBlur (near the hasInteracted assignment) stating that setting
hasInteracted here is intentional for user-driven blurs and that hideCalendar()
is kept pure UI so disconnectedCallback can call hideCalendar() without marking
interaction. Keep hideCalendar() unchanged (no side-effects on hasInteracted)
and leave handleNativeBlur behavior as-is.
| private handleNativeBlur() { | ||
| this.hasInteracted = true; | ||
| } | ||
|
|
||
| private handleCalendarBlur() { | ||
| this.hasInteracted = true; | ||
| this.hideCalendar(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
handleCalendarBlur couples interaction tracking with UI dismissal — consider separating concerns.
handleCalendarBlur sets hasInteracted = true and then calls hideCalendar(). Since hideCalendar() is now a pure UI method (no side-effects on hasInteracted), this coupling is acceptable. However, note that disconnectedCallback → hideCalendar() does not set hasInteracted, which is the correct behavior (no validation on teardown). This asymmetry is intentional but worth a brief inline comment for future maintainers.
🤖 Prompt for AI Agents
In `@src/components/date-picker/date-picker.tsx` around lines 296 - 303,
handleCalendarBlur currently sets hasInteracted = true and calls hideCalendar(),
which couples interaction tracking with UI dismissal; make this intent explicit
by adding a brief inline comment inside handleCalendarBlur (near the
hasInteracted assignment) stating that setting hasInteracted here is intentional
for user-driven blurs and that hideCalendar() is kept pure UI so
disconnectedCallback can call hideCalendar() without marking interaction. Keep
hideCalendar() unchanged (no side-effects on hasInteracted) and leave
handleNativeBlur behavior as-is.
There was a problem hiding this comment.
⚡ 6cbadcd — added inline comment explaining the intentional separation of concerns.
|
@adrianschmidt-bot You can squash the fixups now and rebase on the latest |
adrianschmidt-bot
force-pushed
the
feat/date-picker-required-validation
branch
from
6cbadcd to
7e2e266
Compare
|
Done! Squashed fixups, rebased on latest upstream/main, and updated the commit reference to #3839. ⚡ New commits:
|
When a date picker has the `required` prop set, it will now automatically show an invalid state after the user has interacted with the component and left it empty. The validation is triggered when the calendar closes (hideCalendar), ensuring the field won't appear invalid before the user has had a chance to enter a value. This matches the validation behavior of limel-select. fix: Lundalogik#3839
Wrap multiple JSX elements in the special <Host> element instead of an array literal, per StencilJS coding guidelines.
adrianschmidt
force-pushed
the
feat/date-picker-required-validation
branch
from
7e2e266 to
8d88861
Compare
2 participants
Summary
Adds automatic validation for required date picker fields, matching the existing behavior of
limel-select.How it works
When a date picker has the
requiredprop set, it will now automatically show an invalid state after the user has interacted with the component and left it empty.The validation is triggered when the calendar closes (
hideCalendar), ensuring the field won't appear invalid before the user has had a chance to enter a value. This follows the same pattern used inlimel-select.Changes
hasInteractedstate - Tracks whether the user has interacted with the date pickerisInvalid- Combines the explicitinvalidprop with automatic validation:isInvalid = invalid || (hasInteracted && required && !value)hasInteracted = truewhen the calendar closesdate-picker-required.tsxdemonstrates the validation behaviorUser experience
Fixes #3839
Summary by CodeRabbit
Bug Fixes
New Features
Documentation