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

Form fields error messages are not flashed when validating (still) #697

Closed
tchapi opened this issue Jul 16, 2020 · 13 comments
Closed

Form fields error messages are not flashed when validating (still) #697

tchapi opened this issue Jul 16, 2020 · 13 comments

Comments

@tchapi
Copy link
Contributor

tchapi commented Jul 16, 2020

Following #605 that I cannot reopen, I did some more testing following fc5b16a.

The fields are now correctly highlighted as errored (in red) as expected, but the required fields do not have any text message:

Screenshot 2020-04-21 at 16 08 40

(In this example, both fields have the required|alpha_num requirement)

The return from the API is good though, with the correct error messages:

Screenshot 2020-04-21 at 16 43 16

The culprit is in mixins/inputFrame.js here :

return message.endsWith('is required.') && !this.errorKey.startsWith('block') ? '' : message

I don't really understand why you would silence the messages finishing with 'is required.' — that is the bug here. More over, this is quite dangerous since now the UI is localized ...

@tchapi
Copy link
Contributor Author

tchapi commented Jul 16, 2020

Let me add that this is still the case in Twill 2.1.0 (latest release as of today)

@adam-jones-net
Copy link

adam-jones-net commented Jul 16, 2020

I'm now using Twill 2.1.0 on a site and I don't have this problem on standard fields - I just checked a simple text field that has required defined in the requests file. That works fine with the custom msg appearing exactly as I'd expect. I've tested this site on Laravel 5.8.x and 6.

Capture

However, to muddy the water somewhat, I can't make error msgs show at all within repeaters. I don't know if this has always been problematic or is a recent bug?

I have simple text fields defined in a repeater and I'm positive that my request rules are written correctly. If I leave these fields blank then correctly the update fails with the red bar msg at the bottom of the page. However absolutely nothing occurs inline where the problem input field is. No red msg, no red outline of the field.

@ifox
Copy link
Member

ifox commented Jul 16, 2020

@tchapi that was a silly product requirement at some point, simply because required is kind of explicit when a field is empty and red, but you're totally right that it is confusing, and that it certainly doesn't work with i18n. Feel free to PR to drop it and I will happily release 2.1.1 with it.

@ifox
Copy link
Member

ifox commented Jul 16, 2020

@Arkid I will need to dig deeper for repeaters validation errors display but it is probably not functional and has never been. It should be possible to implement though, no worries.

@adam-jones-net
Copy link

@Arkid I will need to dig deeper for repeaters validation errors display but it is probably not functional and has never been. It should be possible to implement though, no worries.

Ok thanks @ifox , yes it would be really great to have this (any chance to add to roadmap?!), validation seems only half complete if this isn't possible within repeaters.

@ifox
Copy link
Member

ifox commented Jul 16, 2020

I'm definitely adding to the roadmap, however backend validation is functional as you described above yourself, saving is prevented. This is a message display issue in the UI. A simpler solution I'm thinking about that could resolve all of our uses cases would be to group all validation errors at the top or bottom of the form, if we are not able to render errors at every single nested field level. But I'm pretty sure we should be able to!

@adam-jones-net
Copy link

The thing with repeaters is that by their definition, they instantly allow a huge amount of content to be added to a form. So what could otherwise be a very simple form with few fields to manually check suddenly becomes much more likely to be a large form due to repeated elements alone. Therefore for the user to not see any visual red guide highlighting where within the form the problem is makes it quite unfriendly and hard to resolve the error. Hopin it can work nested ;)

@tchapi
Copy link
Contributor Author

tchapi commented Jul 16, 2020

@Arkid yours work because there is no point (.) at the end of the sentence
@ifox let me see how I can fix that quickly !

@tchapi
Copy link
Contributor Author

tchapi commented Jul 16, 2020

See #699
I'm not sure why the CHANGELOG.md file shows differences, though, I haven't touched it... 🤔

@ifox
Copy link
Member

ifox commented Jul 16, 2020

Thank you! I guess because your 2.x forked branch is not up to date. You should be able to rebase your PR to clean it up. I think the condition on the blocks field key is still needed, as block validation is handled differently. I'll try it out. Thanks for the quick contribution!

@tchapi
Copy link
Contributor Author

tchapi commented Jul 16, 2020

Oh, ok, I didn't know that (for the blocks). I did update from the upstream though, so it's bizarre that there is a difference, but well, sometimes I don't understand git fully.

Do you want me to make a clean fork and re-make a PR or are you going to do the change on your own ?

@ifox
Copy link
Member

ifox commented Jul 16, 2020

No worries, I will rebase it before merging. Thanks again!

@adam-jones-net
Copy link

@ifox messages inside repeaters is still outstanding and on the roadmap ?

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

No branches or pull requests

3 participants