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

Removes gotoPage action in favor of setPage #33175

Merged
merged 1 commit into from Mar 13, 2019

Conversation

@crob611
Copy link
Contributor

crob611 commented Mar 13, 2019

Summary

Navigating between pages of a workpad, and then using undo or using the browsers back button to go back would get the UI into a weird state. The ui would update properly, but the url would be unchanged, which would then break trying to navigate back to that page. For example, if you went from /page/1 to /page/2 and then did undo, the url stays /page/2 which makes it impossible to navigate to /page2/ (because to the router, it appears you are already there).

The cause of this issue is that in navigating to a /page/:id url, the router would fire an action that would also do a router navigation, so you were essentially navigating to the same page twice, making it require two undos to get back to where you want to be.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@crob611 crob611 requested a review from elastic/kibana-canvas as a code owner Mar 13, 2019
@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Mar 13, 2019

@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Mar 13, 2019

@cqliu1
cqliu1 approved these changes Mar 13, 2019
Copy link
Contributor

cqliu1 left a comment

LGTM 👍This works as described, and the URL updates correctly without having to undo/redo twice to switch pages. I never noticed the funky behavior with undo/redo and changing pages. Nice catch!

@w33ble w33ble self-requested a review Mar 13, 2019
@w33ble
w33ble approved these changes Mar 13, 2019
Copy link
Contributor

w33ble left a comment

Nice find! That's been a weird issue for a while. LGTM!

@crob611 crob611 merged commit 6b784f5 into elastic:master Mar 13, 2019
2 checks passed
2 checks passed
CLA All commits in pull request signed
Details
kibana-ci Build finished.
Details
crob611 added a commit to crob611/kibana that referenced this pull request Mar 13, 2019
crob611 added a commit to crob611/kibana that referenced this pull request Mar 13, 2019
crob611 added a commit to crob611/kibana that referenced this pull request Mar 13, 2019
crob611 added a commit to crob611/kibana that referenced this pull request Mar 14, 2019
crob611 added a commit that referenced this pull request Mar 15, 2019
crob611 added a commit that referenced this pull request Mar 15, 2019
crob611 added a commit that referenced this pull request Mar 15, 2019
alakahakai added a commit to alakahakai/kibana that referenced this pull request Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.