-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: add error messages to field validation examples #3685
Conversation
9b8cf90
to
53bcc9e
Compare
onValueChanged={(event) => { | ||
value.value = event.detail.value; | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Without the 2-way binding, React resets the value property to the initial value every time the error message is updated, which results in bugs.
const date = dateFnsParse(target.value ?? '', 'yyyy-MM-dd', new Date()); | ||
if (isBefore(date, this.minDate)) { | ||
@validated="${(event: DatePickerValidatedEvent) => { | ||
const field = event.target as DatePicker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a proposal to improve typing for event.target
in CustomEvent-based component events: vaadin/web-components#7868
14c9a08
to
3585586
Compare
this.errorMessage = 'Invalid date format'; | ||
} else if (!field.value) { | ||
this.errorMessage = 'Field is required'; | ||
} else if (field.value < field.min!) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ISO format is lexicographically ordered, so we can use JS comparison operators to compare date strings directly. This simplifies the solution a bit.
const minDate = useSignal(new Date()); | ||
const maxDate = useSignal(addDays(new Date(), 60)); | ||
|
||
return ( | ||
<DateTimePicker | ||
label="Appointment date and time" | ||
helperText="Must be within 60 days from today" | ||
value={format(value.value, "yyyy-MM-dd'T'HH:00:00")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The similar DatePicker and TimePicker examples don't have an initial value, so I decided that it's unnecessary for the DateTimePicker example too. This example seems to already contain a lot due to the validated listener.
minlength="6" | ||
maxlength="12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: These constraints previously didn't apply due to a typo in their property names.
return ( | ||
// tag::snippet[] | ||
<PasswordField | ||
allowedCharPattern="[A-Za-z0-9]" | ||
pattern="^[A-Za-z0-9]+$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this change is that I believe it's better for UX to allow users to enter or paste incorrect passwords. When their entry is completely disallowed, users don't get a chance to see an error message and correct them (using the reveal button).
if (isBefore(date, this.minDate)) { | ||
@validated="${(event: DatePickerValidatedEvent) => { | ||
const field = event.target as DatePicker; | ||
if (!field.value && (field.inputElement as HTMLInputElement).value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a proposal to make the type of inputElement
more specific to eliminate the need for manual casting in listeners: vaadin/web-components#7879
const field = event.target as NumberField; | ||
if ((field.inputElement as HTMLInputElement).validity.badInput) { | ||
this.errorMessage = 'Invalid number format'; | ||
} else if (field.value && Number(field.value) % 0.5 !== 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use validity.stepMismatch
here as we also delegate step
to native <input>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I've updated text field examples to rely on the ValidityState
object where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a follow-up ticket to introduce API similar to HTML ValidityState for our components: vaadin/web-components#7893
frontend/demo/component/passwordfield/password-field-validation.ts
Outdated
Show resolved
Hide resolved
if (!field.value && (field.inputElement as HTMLInputElement).value) { | ||
errorMessage.value = 'Invalid date format'; | ||
} else if (!field.value) { | ||
errorMessage.value = 'Field is required'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could simplify this a bit to only check !field.value
once:
if (!field.value && (field.inputElement as HTMLInputElement).value) { | |
errorMessage.value = 'Invalid date format'; | |
} else if (!field.value) { | |
errorMessage.value = 'Field is required'; | |
if (!field.value) { | |
const notEmpty = (field.inputElement as HTMLInputElement).value; | |
errorMessage.value = notEmpty ? 'Invalid date format' : 'Field is required'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the current approach has a benefit that it keeps all error messages at the same indentation level, which makes them a bit easier to scan.
if (!field.value && (field.inputElement as HTMLInputElement).value) { | ||
this.errorMessage = 'Invalid time format'; | ||
} else if (!field.value) { | ||
this.errorMessage = 'Field is required'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!field.value && (field.inputElement as HTMLInputElement).value) { | |
this.errorMessage = 'Invalid time format'; | |
} else if (!field.value) { | |
this.errorMessage = 'Field is required'; | |
if (!field.value) { | |
const notEmpty = (field.inputElement as HTMLInputElement).value; | |
errorMessage.value = notEmpty ? 'Invalid time format' : 'Field is required'; |
if (!field.value && (field.inputElement as HTMLInputElement).value) { | ||
errorMessage.value = 'Invalid time format'; | ||
} else if (!field.value) { | ||
errorMessage.value = 'Field is required'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!field.value && (field.inputElement as HTMLInputElement).value) { | |
errorMessage.value = 'Invalid time format'; | |
} else if (!field.value) { | |
errorMessage.value = 'Field is required'; | |
```suggestion | |
if (!field.value) { | |
const notEmpty = (field.inputElement as HTMLInputElement).value; | |
errorMessage.value = notEmpty ? 'Invalid time format' : 'Field is required'; |
revert not relevant changes
fcacb55
to
23efedd
Compare
Added error messages to field validation examples using the available APIs in Flow, Lit and React.
Part of #3548