-
Notifications
You must be signed in to change notification settings - Fork 6
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
Transition does not call onUnload #15
Comments
Thanks for reporting this @matthewvalimaki - can you throw together a quick twiddle that shows this behavior for us? here's a twiddle with the addon pre-installed: https://ember-twiddle.com/af013400c74fc407a45e74327ed9f9cb thank you! |
Just a bit off-topic but related to So I think that either README should state that one needs to make sure any code within If we were to change to |
Forgot to mention that |
Likely the README update. The latter would be confusing since But you raise an interesting point, perhaps we need a hook for handling transitions as well. So transitions between routes could be async, but a transitions away from the app needs to be sync. I think a new hook should be introduced for |
The way I understood README was that this was for route transitions as well, so I would prefer your suggestion to introduce new hooks to clarify intent. |
@jasonmit Do you think you'll be able to add |
@dja pretty easy. The logic is in place, just need to split the onUnload hook up into two hooks and call the correct respective hook depending on what occurred. As for help, would love the help :) |
Hi! I wanted to see if this fix was planning on being implemented anytime soon? It seems like that PR above would work for my use case. |
@devups lets get the PR green and I’ll cut a beta. I need to analyze if this is a breaking change. I think this behavior could also be configured so that it’s an opt in instead of all or nothing. How does that sound? |
@tylerturdenpants Yeah, that sounds awesome. Ty! |
@devups Do you mind taking over the PR work? I’m simply too busy. |
@jasonmit first thank you for this trivial looking yet complicated support for detecting when transitioning away from current view. On surface it looks easy but due to browser differences it is a bit pain.
When I navigate to a completely different route (not sub-route) and answer "yes" to the dialog
onUnload
is not called. I believe this is because to the browser it's not unloading anything. I was able to fix this by addingthis.onUnload()
afterember-onbeforeunload/addon/mixins/confirmation.js
Line 98 in 2c8c7bb
else { this.onUnload(); }
.The text was updated successfully, but these errors were encountered: