Fixes cancel button error on Action Sheet #1576
Conversation
The fix that was supposed to close this doesn't: ionic-team@ba39fb0 The real problem was that cancel was being called on opts and not on scope. The code by @ajoslin is redundant as all the three functions he gives a default angular.noop are overloaded 20 lines below so I reverted it.
|
I don't actually understand the difference here. Opts and scope have the same properties in this case. And the functions do need to default to either angular.noop, or actionSheet needs to check if they exist. Defaulting to a noop function is simpler. What problems are you experiencing that this fixes? |
|
Hi @ajoslin, the problem I'm experiencing is the very same the guy reported in #1013. It crashes complaining that opts has no method cancel. The code is first extending scope with your custom object (the one that has all the noops), which is itself extended by opts. So, if I don't provide a cancel parameter, noop will be assigned as cancel to the scope, but not to opts. Later, line 97 calls opts.cancel(), which results in a crash. Hope this clarifies. |
|
As for the part where I removed all the noops, look at lines 118, 122 and 130. It is kind of weird because it seems like the parameters are not being used at all, maybe I should add these callbacks too? |
|
Interesting, I see. It looks like this fix actually presents a different problem. scope.cancel is defined in line 113, which overrides options.cancel. So options.cancel is never called. Could you rename scope.cancel on line 113 to just What this really needs is proper unit testing - which may require a refactor. This crash is unacceptable. |
|
The problem with that is that hideSheet() needs to be called. Look at the commit I just added, should fix both problems. |
Also call the callback if it was provided
|
One more problem: On line 92, hideSheet calls scope.cancel() again. It seems like it would recursively call itself forever. |
remove infinite loop, fix cancel callback, improved comments
|
Fixed. It also seems there was a design problem here. The documentation states the cancel callback will be "Called if the cancel button is pressed or the backdrop is tapped.". Of course now it was not being called at all, but the way I left it would have been called any time the actionSheet disappeared. Additionally, the callback was supposed to be called in a $timeout, I suppose so that when the callback is called the actionSheet really is gone from the screen. I made it that way too now. |
|
By the way, I also think it would make sense to call the cancel callback if the user presses the hardware back button. This would have to be reflected on the docs though. Tell me if you agree and I'll update both the code and the docs. |
|
Yes, sounds reasonable. |
|
All done! |
|
Good work @hallucynogenyc! Thank you. I'll add some real unit tests for action sheet (it's about time) and merge this later today. |
|
Always a pleasure 😄 |
|
Doing this now. |
|
Fixed some other bugs, polished up the code, made the API simpler (with backwards compatibility) and added unit tests in b7646a5 |
The fix that was supposed to close this doesn't:
ba39fb0
The real problem was that cancel was being called on opts and not on scope. The code by @ajoslin is redundant as all the three functions he gives a default angular.noop are overloaded 20 lines below so I reverted it.