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

feature: support form errors (see issue #15) #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bkinsey808
Copy link

What are you adding/fixing?

I love the syntax of ngxErrors so much, that I wanted to use it not just for form control errors, but for form errors itself. See issue #15

Have you added tests for your changes?

I'm sorry, I did not. But the existing tests still passed.

Will this need documentation changes?

Yes, I imagine this feature is something ppl would want to know about.

Does this introduce a breaking change?

I don't think so. As far as I can tell, all existing functionality still remains.

Other information
This code is a bit ugly, not super DRY. I sort of intended it as a proof of concept. It could be cleaned up and refactored, maybe we need a concept of a single object that can be either a form control or a form group. I'm especially displeased with having to call detectChanges() but I haven't found a way around it for OnPush change detection strategy. If anybody knows how to avoid it, pls let me know, I want to learn.

Thanks @toddmotto for a great project!

@listepo
Copy link

listepo commented May 12, 2017

@toddmotto can you check this PR?

@toddmotto
Copy link
Member

We'll get this baked into a future addition of ngxErrors - planning to enhance some of the internals and there's a few things in here which work but what I'd like to do is introduce this feature once we've re-shapen the internals. This will make things a little smoother as there's a few hacks in here to get things to work - we'll add :)

@listepo
Copy link

listepo commented May 16, 2017

@toddmotto thanks

@DmitryEfimenko
Copy link

Also looking forward to this feature.

@picosam
Copy link

picosam commented Sep 2, 2017

Hello @toddmotto would love to know if you've got a timeline for this?

@listepo-alterpost
Copy link

@toddmotto any news?

@JustinElst
Copy link

this looks interesting!

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.

7 participants