Skip to content
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

fix: form-level error reporting #1

Merged
merged 3 commits into from
May 27, 2024
Merged

fix: form-level error reporting #1

merged 3 commits into from
May 27, 2024

Conversation

nhedger
Copy link
Contributor

@nhedger nhedger commented May 25, 2024

This PR fixes an issue where form-level errors were set to the node's name when formLevelErrorName was left unspecified.

Specifying formLevelErrorName will now correctly render a form-level error if an error with the key specified by formLevelErrorName exists in the response payload. Also, leaving formLevelErrorName unspecified will ensure no form-level errors are displayed.

FYI @fenilli

@fenilli
Copy link
Collaborator

fenilli commented May 25, 2024

One question I have tho, this was a feature I implemented out of the specs, but not sure how useful it is, as you're using it do you think it is a useful feature?

Copy link
Collaborator

@fenilli fenilli left a 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 feature was to set a global error message by the form name or another if specified, meaning it should by default be the name of the form, just was not meant to set the name of the field as the error but the global error itself.

Something like if you have a global error in laravel, you can just pass the name of the form field in the errors field and it would display there.

node.setErrors(hasGlobalError ? formErrorMessage : [], errors) should be this.

@nhedger
Copy link
Contributor Author

nhedger commented May 25, 2024

One question I have tho, this was a feature I implemented out of the specs, but not sure how useful it is, as you're using it do you think it is a useful feature?

I never use form-level errors, but I can see how someone might want to show one. When using Form Requests with Laravel, you usually attach errors only to the fields though.

@nhedger
Copy link
Contributor Author

nhedger commented May 25, 2024

The reason for this feature was to set a global error message by the form name or another if specified, meaning it should by default be the name of the form, just was not meant to set the name of the field as the error but the global error itself.

Something like if you have a global error in laravel, you can just pass the name of the form field in the errors field and it would display there.

node.setErrors(hasGlobalError ? formErrorMessage : [], errors) should be this.

Not sure I'm following.

My understanding was that by setting formLevelErrorName, you're essentially telling the useForm helper to hoist one of the field validation errors to the form level.

For example, say you have a login form, and submitting the form with invalid credentials would return the following error bag:

{
  "email": "Invalid credentials"
}

You would set formLevelErrorName to email to render Invalid credentials at the form level instead of under the email input.

@fenilli
Copy link
Collaborator

fenilli commented May 25, 2024

That was not the reason but it could be too, but I would say for this its better to use other ways directly into the FormKit.

The thing I meant is is there an error like "The user was not created, call your IT". It is a global error.

{
  userForm: 'The user was not created, call your IT',
}

<FormKit type="form" name="userForm">
</FormKit>

@nhedger nhedger requested a review from fenilli May 27, 2024 11:29
@nhedger
Copy link
Contributor Author

nhedger commented May 27, 2024

I believe this should do it.

@fenilli
Copy link
Collaborator

fenilli commented May 27, 2024

This looks good I will work on it to publish a new version.

Copy link
Collaborator

@fenilli fenilli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@fenilli fenilli changed the base branch from main to fix/form-level-error May 27, 2024 16:52
@fenilli fenilli merged commit 03ce3f9 into formkit:fix/form-level-error May 27, 2024
@nhedger
Copy link
Contributor Author

nhedger commented May 27, 2024

Thanks !

@fenilli
Copy link
Collaborator

fenilli commented May 27, 2024

This will be fixed after publishing 0.1.6.

@nhedger nhedger deleted the form-error branch May 27, 2024 17:10
@nhedger
Copy link
Contributor Author

nhedger commented Jun 21, 2024

Hey @fenilli, not meaning to press, but are you planning to release 0.1.6 soon-ish? No biggie if you don't have time, I can definitely fork in the meantime.

@fenilli
Copy link
Collaborator

fenilli commented Jun 21, 2024

@justin-schroeder did you publish to npm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants