Skip to content
This repository has been archived by the owner on Jul 1, 2020. It is now read-only.

Get form within scope #15

Merged
merged 3 commits into from
Mar 31, 2015
Merged

Get form within scope #15

merged 3 commits into from
Mar 31, 2015

Conversation

tlastad
Copy link
Contributor

@tlastad tlastad commented Mar 30, 2015

Previous code retrieved first form on page, and exited if that form did not have a name.
This change will let it get first form with a name within scope.
Needed if, like in my case, there are several nameless forms on the page that are out of my control and out of scope.

Previous code retrieved first form on page, and exited if that form did
not have a name.
This change will let it get first form with a name within scope.
Useful if, like in my case, there are several forms on the page that are
out of my control.
@ghiscoding
Copy link
Owner

Oh yes you are right, I assumed that most people would have a name on the Form.
I will try your code tonight after work, it looks promising, just a question though, you loop through all the forms on the page, how will you know that you are on the good form? What if you don't have validations on the first form, will it be that one returned anyway? I think that I actually overlooked that fact myself though.

Your indentation seems a little offset at some areas, it might have to do 2 commits to fix that unless you want to fix it.

Thanks for the help

fixed offset indents
@tlastad
Copy link
Contributor Author

tlastad commented Mar 30, 2015

I guess I don't really know, I just used my case as a sample.
I am a part of a team that develops a rather large application, and as such I am mostly developing on the "content" part of each page.
I my case we have a couple of search boxes that form posts their queries, and are in reality in another project.
What we do have is control of the "content" part, and that is where we define angular-app and controller. Within that context there's only one form.
So what I tried to do was get the first form with a name that was within the scope of the app.

The indentations should be fixed now.
Or not... seems like my editor and git doesn't like each other very much.... :)

should be spaces, not tabs
@ghiscoding
Copy link
Owner

Oh wow, this is nice knowing that my code is now used on large application, it's good for the ego hehe. I will probably push your code as is, since in my cases I also develop with the idea of 1 form per page so that wouldn't affect my side either. Unless we verify that there is at least 1 "validation" attribute in the list of elements that belongs to the form that your getScopeForm() is finding, would that be doable?

Again thanks a lot for the help, really appreciate it, which is also why I made it open to the big community of Open Source :)

@tlastad
Copy link
Contributor Author

tlastad commented Mar 30, 2015

I don't see why not...
Could you please fix that? You have a much better understanding of the code than I do.
If you can't find the time, I'll give it a go tomorrow.

@ghiscoding
Copy link
Owner

Alright, I made some more code changes, added and tested that it works properly. To make sure that the form you found (with your code) really has children with validation attribute, I ended up adding a new function (I cannot seem to be able to do it in 1 line of code because of jqLite limitation, hence the new function):

/** Make sure the Form has at least 1 child element with a validation attribute 
 * @param object form
 * @return bool
 */
function hasValidationAttributes(form) {
  for(var i = 0, ln = form.elements.length; i < ln; i++) {
    if(!!form.elements[i].getAttribute('validation')) {
      return true;
    }
  }
  return false;
}

and finally modified your line 292 with an extra call to the new function

if (form && form.name && self.scope[form.name] && hasValidationAttributes(form)) {

If you could make all your tests at your work and make sure that everything works as intended, I would be happy to have a revised pull request including the piece that I added.

As a side note, I found and fixed a little bug in my new checkFormValidity() function that I recently added. It doesn't affect your pull request (since it's in the service file), but I found it while I was trying and coding with your changes. I did not stamp it as a new revision as it's a minor tweak.

Thanks again

@ghiscoding
Copy link
Owner

Actually forget my code, I just found out that it breaks my other page when using Form with only the Service since the html doesn't have validation attributes. So I will accept your code as is, I will have to find another way on making sure it's the same form.

ghiscoding added a commit that referenced this pull request Mar 31, 2015
@ghiscoding ghiscoding merged commit 5c83d91 into ghiscoding:master Mar 31, 2015
ghiscoding added a commit that referenced this pull request Mar 31, 2015
- Recompiled minified script after pull request #15
@ghiscoding
Copy link
Owner

Hi there, I just want to let you know that I had to redo and replace the getScopeForm(self) by a new function of getElementParentForm(self) to fix the issue #23

I hope this will not break your code but since I don't have a way to test your code, I prefer to advise you... Let me know if something is broken on your side. I tried on my side with/without names on the form and it seems all good. Prior to this change, the $validationSummary was never really separated, it was always having the errors of all the forms, now it's fixed. You can see the example with multiple form on my main page demo.

Try it out. Hopefully it's still all good.

@tlastad
Copy link
Contributor Author

tlastad commented Apr 9, 2015

Thanks for the heads-up.
Check out my latest pull request :)

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

Successfully merging this pull request may close these issues.

2 participants