Forms signup improvements 1 #244
Conversation
…entation. * potentially, you could have a pre-filled sign-up form where `SignupForm::default` wouldn't be used, but the process of building a form should be the same.
… in tuples or individually. * where variables are frequently/always used together it indicates a structure should be used to hold them.
…world by making it return error codes.
hydra
force-pushed
the
forms-signup-improvements-1
branch
from
f53bde2 to
04eef7f
Compare
…ors are always notified. * If `set` is used, and a newly calculated `mapped_errors` is exactly the same as the last time they were calculated, then subscribers are not notified resulting in incorrect form behavior because `set` uses `PartialEq` on the value before notifying subscribers. This manifests in a bug where the UI shows no error messages when the API is used. To re-create the issue, use a value for the username which generates an API error, then in the UI clear the field, then enter the same value again and re-submit, no error was shown on the second submit even though there was an API error.
| @@ -63,6 +63,7 @@ nominals = "0.3.0" | |||
| parking_lot = "0.12.1" | |||
| easing-function = "0.1.1" | |||
| serde = { version = "1.0.210", features = ["derive"], optional = true } | |||
| regex = "1.11.1" | |||
There was a problem hiding this comment.
Dependencies should not be added specifically for examples.
| .persist(); | ||
| #[derive(Default)] | ||
| struct SignupFormFieldState { | ||
| username: Dynamic::<String>, |
There was a problem hiding this comment.
This PR needs cargo fmt to be run against it.
| @@ -313,14 +377,37 @@ struct FakeApiRequest { | |||
|
|
|||
| #[derive(Debug)] | |||
| enum FakeApiResponse { | |||
| SignUpFailure(Map<SignupField, String>), | |||
| // the API returns numbers, which needs to be mapped to a specific error message | |||
| SignUpFailure(Vec<u32>), | |||
There was a problem hiding this comment.
Why would a Rust API use u32's instead of an enum to represent the error? I think by introducing this extra step this is complicating the example more than it needs to.
| } | ||
| if username == "user" { | ||
| errors.push( | ||
| FakeApiSignupErrorCode::UsernameUnavailable.into(), |
There was a problem hiding this comment.
What does this extra if statement provide that the existing admin one didn't? Why are we complicating this example with extra arbitrary errors rather than keeping it as simple as possible to minimize confusion.
| }, | ||
| Ok(FakeApiSignupErrorCode::UsernameInvalid) | ||
| => { | ||
| mapped_errors.insert(SignupFormField::Username, String::from("Username is invalid")); |
There was a problem hiding this comment.
If we're going to include this as an example, error messages should contain the information required to pass validation.
| PasswordInsecure = 69, | ||
| } | ||
|
|
||
| impl TryFrom<u32> for FakeApiSignupErrorCode { |
There was a problem hiding this comment.
Why are we doing this instead of using something like Serde?
See individual commit messages.
Based on feedback in discord: https://discord.com/channels/578968877866811403/1334602387750518806/1336400179783536651