Update localization support to include form hints, validations and placeholders.#243
Update localization support to include form hints, validations and placeholders.#243hydra wants to merge 2 commits into
Conversation
|
I just remembered I still need to add |
hydra
changed the title
7497c10 to
f0f9ecc
Compare
|
rebased |
… for testing localized validation support.
f0f9ecc to
34f63ce
Compare
|
rebased again. |
ecton
left a comment
ecton
left a comment
There was a problem hiding this comment.
In addition to the existing comments, it looks like this PR also needs cargo fmt to be run against it. Additionally I suspect running cargo clippy will return warnings that should be addressed too.
|
|
||
| impl<T> PartialEq for Value<T> | ||
| where | ||
| T: PartialEq + Clone |
There was a problem hiding this comment.
This shouldn't require Clone, and I also don't like that Dynamic's implementation of PartialEq is different than Value::Dynamic's PartialEq result. To me PartialEq should work the same for a wrapped value as the wrapper in this situation.
| Valid, | ||
| /// The data is invalid. The string contains a human-readable message. | ||
| Invalid(String), | ||
| Invalid(Value<MaybeLocalized>), |
There was a problem hiding this comment.
Why is this a Value rather than just a MaybeLocalized? Have you made sure that everywhere this is used that the contained value is properly being tracked to ensure external updates invalidate where it is being used?
| /// validation status will include this validation. | ||
| #[must_use] | ||
| pub fn validate<T, E, Valid>( | ||
| pub fn validate<T, Valid, R>( |
There was a problem hiding this comment.
Why was this changed from E to R? E stands for Error, I don't know what R would stand for.
2 participants
Recording.2025-01-14.142838.mp4
Notes for reviewers: