Skip to content

$viewChangeListeners solution for #135 #155

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

Closed

Conversation

jamesbrobb
Copy link
Contributor

Heres the refactor of my previous solution for #135 that uses $viewChangeListeners instead of adding an ng-change attribute.

I've separated out the validateMultipleFields directive into a directive and controller. Each ngModelController for the ngForm is then passed to the ValidateMultipleFieldsCtrl by an input directive, as they're are issues when trying to directly access them within the validateMultipleFields directive.

For example, with the 'sex' radio boxes in the example, if you attempt to access them within the validateMultipleFields directive, as they both have the same name, you end up accessing the same ngModelController twice on the ngFormController. Which results in multiple $viewChangeListener functions being added to the same ngModelController.

Where as if you pass them from their own input directive, they're two seperate ngModelController instances.

I hope that makes sense.

I've tested it in FF, chrome and safari and it works as expected.

Let me know what you think?

@jamesbrobb
Copy link
Contributor Author

Just to note, another advantage to this solution, is that it removes the need to supply the validate-multiple-fields directive with the names of the sub-fields it needs to manage.

The logic for this still remains intact, but if you supply nothing, then it just allows all existing ngModel directives under the validate-multiple-fields directive to be added to the controller. If a list of sub-fields is supplied, then only ngModel's whose name match, are added to the controller.

@jamesbrobb
Copy link
Contributor Author

#160 offers an alternative fix for this issue

@adrienbrunet
Copy link
Collaborator

@jamesbrobb While still really interesting, this issue has been fixed some time ago. Closing this. #160 offers about the same thing (in terms of fix). But maybe some refactoring in it are worth the read. I let it open for now

@jamesbrobb
Copy link
Contributor Author

this issue has been fixed some time ago.

@adrienbrunet i should hope so, this PR is is nearly a year old :)

You should definitely take time to thoroughly read and understand #160. At the time it was written (also a while ago) it fixed several flaws in the original code, as although not explicitly failing, it was not working in the manner expected. I have no idea whether any of those issues have since been rectified.

You also need to understand that all of my PR's where submitted quite a while ago when i was actually using django-angular. I'd hoped to make a positive contribution to the project, but unfortunately most of them either sat unanswered or after initial responses where never merged. Read in context, they made sense, but that might not be the case 12+ months on.

@adrienbrunet
Copy link
Collaborator

Yep, for sure, a lot of it is no more relevant but it's still really interesting and it's very good work IMO. At least, I can learn a lot from it. Thanks a lot for that. Feel free to come back using this library, we might need your help :D haha

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.

2 participants