Add onbeforeunload support #97
Conversation
|
CLA is valid! |
|
Greg pinged me on IM and mentioned that he'll have a similar PR for |
|
@jedireza Just sent the fluxible-router PR |
…gate to a different page
|
I don't totally understand this use case, but I'm not a fan of handling this in the action. Ideally the components are the only things that deal with |
|
@mridgway This handles the following use case:
Currently, the standard is to set a |
… mixin and the NavLink component per @mridgway
|
Updated the PR to handle cases for both the |
|
👍 looks good to me. I want @lingyan to take a look before merging. |
|
|
||
| if (!confirmResult) { | ||
| // Pushes the previous history state back on top to set the correct url | ||
| self._history.pushState(historyState, pageTitle, (state.route.url || url)); |
There was a problem hiding this comment.
Using url as fallback is debatable, since it is going to be the new url after history popstate.
Another tricky thing is about the scroll position, if user set enableScroll to true for RouterMixin, RouterMixin saves the scroll position in history state. In this case of canceling navigate, I wonder if we should add the current scroll position to the historyState object, if enableScroll is true.
|
@gfranko Thanks for your contribution. Could you add some docs about this? |
|
@lingyan Yea, I'll add some docs and update the |
|
@lingyan Updated per your comments. Let me know if there is anything else I should update! |
| }; | ||
| var pageTitle = navParams.pageTitle || null; | ||
|
|
||
| if (!confirmResult && currentUrl) { |
There was a problem hiding this comment.
I don't think currentUrl would be undefined or empty. This && currentUrl adds confusion to the if/else logic, since it does not really make sense to continue with navigation if user has said no (i.e. !confirmResult). I would just remove && currentUrl.
If you are worried about it being falsy, I'd rather check for it inside the if clause and throw an error, since it is not supposed to be that way.
|
I also sent a PR to fluxible-router with all of these changes. |
|
@gfranko Looks great 👍 I'm merging now. Thanks for your contribution! |
|
@lingyan Awesome, thanks! |
|
New patch version released: https://github.com/yahoo/flux-router-component/releases/tag/v0.6.3 |
|
👍 |
Add onbeforeunload support Conflicts: lib/NavLink.js
This handles the situation when a user leaves a particular page and the
window.onbeforeunload()method needs to be called.I had originally created an issue on the fluxible-router repo, but we need this change for both repos.