Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

client-side form validation with ng-messages. #759

Merged
merged 1 commit into from
Aug 25, 2015

Conversation

rhutchison
Copy link
Contributor

Add validation on the client to validate form data prior to posting to the server.

@ilanbiala
Copy link
Member

@rhutchison @lirantal not directly about the PR itself but I think we need to add to 0.5.0 conventions another item about refactoring all of the Angular code to remove the data- prefix. That was only necessary for XHTML and it just makes the code longer and seems unnecessary.

If you guys agree, then we should already start enforcing that with new PRs and refactor the rest before we release next time. We could even do it for 0.4.x because it wouldn't break anything, it's just a style change really.

@rhutchison
Copy link
Contributor Author

@ilanbiala I agree with removing it, but I would like to do a single PR for that. We can do it before this is merged or after - up to you guys.

@mleanos
Copy link
Member

mleanos commented Aug 6, 2015

+1 on removing the data- @rhutchison About when to remove it, I don't think it's detrimental either way. I think encouraging (enforcing) this style change right now, for any new PR's, could be helpful though.

@mleanos
Copy link
Member

mleanos commented Aug 6, 2015

LGTM. I've tested this, and it works really well. I really like the implementation, as it's easy to add to any form.

I would like to share a possible limitation, or tricky use case though...

I tried playing around with ng-messages by adding validation to the Chat Example. This didn't function quite right. The sendMessage() method clears out the messageText field, after a successful form submit, and the ng-messages validation see's this as an invalid form. I fiddled with it quite a bit, and couldn't get it to function properly. However, I do realize that the Chat Example isn't a very good use case for this feature; the submit button here should be disabled unless the form is valid. For most use cases, ng-messages seems to work really well. It might be tricky when you have a form that can have an empty field after a valid form submit. Perhaps, this is something to watch out for.

Overall, this feature really enhances the UI. Thanks @rhutchison

@ilanbiala
Copy link
Member

@rhutchison if possible enforcing for all new PRs will be best, and then 1 PR at the end before a release to fix all the existing/remaining ones so we don't have to keep rebasing and doing that stuff like we had to do with the spacing PR.

@rhutchison rhutchison force-pushed the client-side-validation branch 3 times, most recently from ecc6b29 to e551ed7 Compare August 8, 2015 04:01
@rhutchison
Copy link
Contributor Author

closing to reopen/rerun test

@rhutchison rhutchison closed this Aug 8, 2015
@rhutchison rhutchison reopened this Aug 8, 2015
@lirantal lirantal added this to the 0.4.x milestone Aug 8, 2015
@rhutchison
Copy link
Contributor Author

@ilanbiala

@ilanbiala
Copy link
Member

I'll have to review when I'm at a computer. If I don't get to this in a week or so, just remind me because I'm on vacation for the next week and I probably won't be near a computer.

@rhutchison rhutchison force-pushed the client-side-validation branch from bc00951 to 0c70f5f Compare August 17, 2015 03:02
@codydaig
Copy link
Member

Once data- is put back in for consistency (at least for this PR), it will LGTM.

@lirantal
Copy link
Member

so are we in favor of removing data-* or keeping it?

@ilanbiala
Copy link
Member

I'm in favor of removing.

@codydaig
Copy link
Member

+1 for removing.

@rhutchison
Copy link
Contributor Author

Closes/supersedes #247

@rhutchison rhutchison mentioned this pull request Aug 20, 2015
15 tasks
@mleanos
Copy link
Member

mleanos commented Aug 21, 2015

+1 for removing data-

remove data prefix from attributes.

fix tests
@rhutchison rhutchison force-pushed the client-side-validation branch from 194b7d1 to 8015476 Compare August 25, 2015 06:02
@mleanos
Copy link
Member

mleanos commented Aug 25, 2015

Just tested this with the most recent changes.. Everything seems to be working.

LGTM.

@lirantal lirantal assigned lirantal and unassigned ilanbiala Aug 25, 2015
@lirantal
Copy link
Member

Good, thanks.

lirantal added a commit that referenced this pull request Aug 25, 2015
client-side form validation with ng-messages.
@lirantal lirantal merged commit 01bd98b into meanjs:master Aug 25, 2015
@rhutchison rhutchison deleted the client-side-validation branch August 26, 2015 04:35
@ilanbiala ilanbiala modified the milestones: 0.4.1, 0.4.x Aug 31, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants