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 BREAD Validation issues #1963 and #2179 #2660

Merged
merged 5 commits into from Feb 16, 2018

Conversation

blitux
Copy link
Contributor

@blitux blitux commented Feb 13, 2018

Hello maintainers,

This is a cleaned up version of an PR I've sent before that fixes two BREAD validation issues:

Fixes #1963: Validation scroll to first error not work
Fixes #2179: Validation fails when display name and field name of a BREAD field don't match

Closes #2612

This uses custom attributes to show $field->display_name on validation error messages, and that's why I'm reverting #2077.

I hope this will be included in the next release as, currently, BREAD form validation is broken when display_name and field_name don't match.

Thank you in advance.

@blitux blitux force-pushed the fix/field-validation-1963-2179 branch 2 times, most recently from d60c40a to 5f7f94c Compare February 13, 2018 17:22
@blitux blitux force-pushed the fix/field-validation-1963-2179 branch from 5f7f94c to dd2dad4 Compare February 13, 2018 17:23
@fletch3555
Copy link
Collaborator

Looks like it should be good. I don't have time to test it, but perhaps @emptynick or @akazorg does. If one of them say it's good, I'll get this merged in.

Thanks!

@regiszanandrea
Copy link
Contributor

Part of this PR is related to PR #2612.

@fletch3555
Copy link
Collaborator

@regiszanandrea, can you be more specific? Both PRs touch ~20 files, so it's a royal pain to truly compare them. What part of this is related to #2612?

@regiszanandrea
Copy link
Contributor

regiszanandrea commented Feb 15, 2018

@fletch3555 This part: "Validation fails when display name and field name of a BREAD field don't match". The only difference is that @blitux use JS. Personally, I think this PR, better, because it resolves two problems. If you accept this PR, you should close the #2612.

@vblinden
Copy link

This indeed fixes the problem. This also looks good too me @fletch3555.

@fletch3555 fletch3555 merged commit f6e0a10 into thedevdojo:1.0 Feb 16, 2018
@blitux
Copy link
Contributor Author

blitux commented Feb 16, 2018

Awesome guys, thank you for your time!

@blitux blitux deleted the fix/field-validation-1963-2179 branch February 16, 2018 15:28
@blitux
Copy link
Contributor Author

blitux commented Feb 16, 2018

@fletch3555 @samtheson The PR 2198 should also be closed

@fletch3555
Copy link
Collaborator

Good catch, thanks @blitux

@gausam
Copy link
Contributor

gausam commented Feb 16, 2018

@fletch3555 @blitux Hey gents. Sorry was away on a long holiday and then came back to tight work in January. Back to a normal life starting next week. Thanks for all the work on this.

@fletch3555
Copy link
Collaborator

No problem, welcome back!

@gausam
Copy link
Contributor

gausam commented Feb 17, 2018

Thank ye!

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

Successfully merging this pull request may close these issues.

None yet

5 participants