-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: add i18n error message support for TextField #6343
Conversation
7b20ca4
to
7361585
Compare
24eb2a5
to
1f71052
Compare
* | ||
* @return the error message or {@code null} if not set | ||
* @see TextField#isRequiredIndicatorVisible() | ||
* @see TextField#setRequiredIndicatorVisible(boolean) |
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.
question: Should we mention isRequired()
and setRequired(boolean)
here as well?
96bb60e
to
3f8e7d5
Compare
|
||
ValidationResult maxLengthResult = ValidationUtil | ||
.validateMaxLengthConstraint(getMaxLengthErrorMessage(), value, | ||
hasMaxLength() ? getMaxLength() : null); |
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: getMaxLength()
returns 0
if the maxLength
property is null
. I added this check to ensure the max length is only considered when the user has explicitly set it with setMinLength(int minLength)
.
...adin-flow-components-base/src/main/java/com/vaadin/flow/component/shared/ValidationUtil.java
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
ValidationResult maxLengthResult = ValidationUtil |
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 wonder if it would make sense to reuse validateMaxConstraint
?
ValidationResult maxLengthResult = ValidationUtil.validateMaxConstraint(
getMaxLengthErrorMessage(), value.length(),
hasMaxLength() ? getMaxLength() : null);
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.
Hmm, I didn’t think about that. Thanks. I personally don't have a strong opinion on which way is better.
To keep it consistent, I guess we should also use validateMinConstraint
for minLength
. If we use it, we will need to add a check outside the validator to see if the value is an empty string to not run the validator then.
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 don't have a strong preference for either approach so this isn't a blocker for the PR
Quality Gate passedIssues Measures |
50091ff
to
5911e46
Compare
checkRequired => validateRequiredConstraint add unit test polish run formatter docs: improve grid empty state documentation (#6351) add more unit tests wip
5911e46
to
9b255ba
Compare
private String getRequiredErrorMessage() { | ||
return Optional.ofNullable(i18n) | ||
.map(TextFieldI18n::getRequiredErrorMessage).orElse(""); | ||
} | ||
|
||
private String getMinLengthErrorMessage() { | ||
return Optional.ofNullable(i18n) | ||
.map(TextFieldI18n::getMinLengthErrorMessage).orElse(""); | ||
} | ||
|
||
private String getMaxLengthErrorMessage() { | ||
return Optional.ofNullable(i18n) | ||
.map(TextFieldI18n::getMaxLengthErrorMessage).orElse(""); | ||
} | ||
|
||
private String getPatternErrorMessage() { | ||
return Optional.ofNullable(i18n) | ||
.map(TextFieldI18n::getPatternErrorMessage).orElse(""); | ||
} |
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 use the same approach as suggested in #6365 (comment)
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.
Thanks, updated.
During our internal discussion, we discovered a way to keep error messages that are set with |
|
||
private boolean manualValidationEnabled = false; | ||
|
||
private String customErrorMessage; | ||
private String constraintErrorMessage; |
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.
What do you think about extracting this somehow? There could be a class that:
- Manages custom + constraint error message states
- Updates the effective error message property on the component when either is set
It wouldn't do a whole lot, but it looks like you have to replicate the logic quite a few times for different components. There's also checkValidity
which is always the same though that one needs to access more stuff from the component. Anyway, maybe something to think about, we can also do this after updating all components and then see if there is something that works for all of them.
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.
Are you suggesting extracting this only for text fields or for all field components?
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 guess we could have something like ErrorMessageController
:
class ErrorMessageController {
private Component component;
public ErrorMessageController(Component component) {
this.component = component;
}
public void setCustomErrorMessage() {
...
this.updateErrorMessageProperty();
}
public void setConstraintErrorMessage() {
...
this.updateErrorMessageProperty();
}
private void updateErrorMessageProperty() {
this.component.getElement().setProperty("errorMessage");
}
}
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.
There's also checkValidity which is always the same though that one needs to access more stuff from the component.
I discussed this with @yuriy-fix some time ago, and we concluded that it would currently complicate things especially since the whole i18n would need to be passed to that helper class or the class would need to provide an API to abstract itself from i18n. We agreed to revisit this in the future when we get to implementing error message providers.
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.
Yep, something like what you suggested with the controller. It seems like that would be applicable to all components, but not sure. But since there are other potential opportunities for code reuse, let's maybe finish changing the components first, and then see what we can extract as common logic?
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.
Agreed 👍
...rent/vaadin-text-field-flow/src/main/java/com/vaadin/flow/component/textfield/TextField.java
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
This ticket/PR has been released with Vaadin 24.5.0.alpha6 and is also targeting the upcoming stable 24.5.0 version. |
Description
The PR introduces TextFieldI18n with methods for setting error messages and updates the TextField's validation logic to show these error messages when validation fails.
Note
It's still possible to use the
setErrorMessage
method to configure a single static error message. This error message will be displayed for all constraints and will take priority over any i18n error messages that are also set. However, when more than one error message is needed, i18n should be used or manual validation mode should be enabled.Depends on
Part of #4618
Type of change