Skip to content
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

Fix for #2 (confirmation on browsers back button) #30

Closed
wants to merge 15 commits into from

Conversation

piotr-cz
Copy link
Contributor

@piotr-cz piotr-cz commented Jun 6, 2018

This and attempt to fix #2 and revert of af559d2.

Additionally

  • it seems that somehow history.goBack() executes after history.block so I've added the timeout
  • due to timeout, component may be trying to use setState after it has been unmounted so I've added simple check

Please do not merge yet

@ZacharyRSmith
Copy link
Owner

I don't think this will work for reasons stated in #2 . I do not think goBack can be supported without memory history

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Jun 9, 2018

With this PR it works with browserHistory, however not yet with hashHistory

@piotr-cz
Copy link
Contributor Author

piotr-cz commented Jun 9, 2018

Should work for both cases now.
I realize that this is a hack, but this is all I have at the moment.

BTW: while reading the history docs, I can't find it mentioning that the history.goBack method works only for memoryRouter

@ZacharyRSmith
Copy link
Owner

Yes, the history.goBack method does work for memoryRouter, just not well, as illustrated here:

If user has this in their history:

/1/path/ago
/2/paths/ago
and right click on back arrow and clicks on /2/paths/ago, then pop() would take them to /1/path/ago instead of /2/paths/ago

here is the same in pictures:

screen shot 2018-06-17 at 10 54 50 am

screen shot 2018-06-17 at 10 55 02 am

Ie, goBack() is not sufficient when the user goes back by more than 1. Ie, when the user goes back, we need to know how far they are going back, and pass that to go()...but we cannot know how far the user is going back without memoryHistory.

However, if you want to adjust this PR so that using goBack happens only when a param is passed to to NavigationPrompt telling it to use goBack, then I'll merge. I just don't want goBack to be the default because of its surprising behavior when the browser tries to go back by more than 1.

@piotr-cz
Copy link
Contributor Author

Thanks, I understand now.

Use case for adding a param to use goBack instead of push method is in mobile app (progressive web app/ webview/ cordova) when it's possible to navigate trough history only one entry back: either by using hardware back button or immersive mode back button.

@piotr-cz
Copy link
Contributor Author

I've messed my branch really badly after rebase so this PR is superseeded by #39 which contains better description too.

Thanks for the screenshots, these were very helpful to understand how you see things.

@piotr-cz piotr-cz closed this Sep 30, 2018
@piotr-cz piotr-cz deleted the patch-2 branch October 10, 2018 07:52
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.

Perform back from browser, but onConfirm didn't perform back
2 participants