search

facultymatt / angular-unsavedChanges

angular-unsavedChanges
175 stars 81 forks source link

Bunch of changes for reuse and developer convenience #4

Closed devorbitus closed 10 years ago

devorbitus commented 11 years ago

Remove need for unsavedWarningGroup and replaced with unsavedWarningSharedService behind the scenes. Added unsavedChangesConfigProvider to set properties at the per project level. Added ability to configure confirm messages dynamically through config provider as well as ability to use the angular-translate service to convert the messages to another language if the module is being used. Added the ability to NOT use the angular-translate service even if it is setup if desired. Added ability to enable and disable logging through config provider.

facultymatt commented 11 years ago

Thanks for this PR! So much great work! I'll review within the next few days.

facultymatt commented 10 years ago

OK, first thing I notice is that clicking "disregard changes" only works first time. Subsequent times the prompt doesn't appear. These little things would be easier to catch with proper unit tests... I'm going to work on that now and then, if you could refactor your PR to pass the tests, that would be great. I can add additional tests for your new functionality... I just want to make sure the base stuff still works.

devorbitus commented 10 years ago

I wasn't using the disregard changes button as I was using the angular form infrastructure to reset the form to a clean state, namely the $setPristine() function on the form controller so I have now modified the disregard changes button to work the same way. Instead of removing event handlers and reapplying them we let Angular be the final say in what we care about by letting Angular know that we consider this form no longer dirty both on the disregard changes button and on submit. Subsequent changes will be detected normally and the disregard button would be bound to that specific form instance. This would "fix" your newest known issue on your feature branch where pressing the disregard changes button removes ability to confirm in the future. With this change a developer would not be able to remove all bindings but I would argue the need for full removal of all event handlers would most likely not be a valid use case as they would need to be reapplied later for full value without an easy way to do that.

Working within the Angular form controller architecture also "fixes" your second to last known issue of submitting one form clears alerts for all future forms as the event handlers are never removed.

facultymatt commented 10 years ago

@devorbitus Thanks again for making these updates to the PR. It makes sense to rely on Angular as the "final say" and to leverage the form controllers dirty flag. This makes things a lot cleaner.

As you likely noticed I setup end to end tests in another branch. I pulled your branch, cherry picked the test commits, and guess what, they all pass!!! (after adding in a few extra ID's for element selection)

Rather then asking you to merge in the tests into this branch, I'm going to merge everything into develop and make the ID changes myself.

Thanks again for this PR, it really makes the module much more robust!