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 back button bug in settings app #6011

Closed
wants to merge 1 commit into from
Closed

fix back button bug in settings app #6011

wants to merge 1 commit into from

Conversation

michaelcheng924
Copy link

This code essentially mimics the way the main tabs work. If we'd like to figure out a way to share the url function code between the different settings tabs, let me know. Just wanted to see if the direction of this possible solution is acceptable first. Closes #5982.

backsettings

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@Bargs
Copy link
Contributor

Bargs commented Jan 26, 2016

This fixes the tabs but not the links inside any given page:

backbutton

I'm wondering if there's a more universal way to solve this problem. Your code made me realize the issue had something to do with the way global state was managed. I tracked it down to the following line in this commit: 7008a51#diff-e82b59fbbd4c2cea495b7da43442f812R82

Because the hrefs in the page don't include any query params, $location.search().replace() is used which breaks the back button. I don't know why this change was made, I can't find a PR associated with the commit that changed it, so @spalger might have to enlighten us.

@michaelcheng924
Copy link
Author

@rashidkpc This probably won't be the final solution. Just waiting on an update concerning @Bargs's observation.

@Bargs
Copy link
Contributor

Bargs commented Feb 11, 2016

Need @spalger's input on why replace is being used.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@spalger
Copy link
Contributor

spalger commented Apr 5, 2016

jenkins, test it

@spalger
Copy link
Contributor

spalger commented Apr 5, 2016

While looking into this I discovered the root cause and took care of it in #6788

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants