-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 login page fragment handling after soft reload on Firefox #353
Conversation
(Rebased to resolve conflicts.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know Javascript all that well, but I think this is ok
I'm gonna approve but I'd like an ack from another maintainer before we merge
Thanks @ffdybuster
(Rebased to resolve conflicts.) |
templates.go
Outdated
var idx = inputs[i].value.indexOf('#'); | ||
if (idx >= 0) { | ||
inputs[i].value = inputs[i].value.substr(0, idx); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding a brief comment to outline what happens here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I added comments, I hope it's now easier to understand!
Apologies @ffdybuster, because we have recently cut a release, the changelog entry you've added is in the wrong place now, if you could move it up to the new heading, then I'm happy to get this merged 👍 |
@JoelSpeed thanks, I overlooked that it ended up in the wrong place. I've updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ffdybuster, LGTM!
@syscll I'll let you do the honours
(Totally not your fault on the changelog btw, we move a lot of stuff as we cut releases so the first few PRs in always have to have the entries moved)
@JoelSpeed @syscll thanks for reviewing this! |
Description
Fixes #352.
Motivation and Context
Soft reloads on Firefox screw up the redirect link on the sign in page; see #352 for details.
How Has This Been Tested?
I've downloaded the generated sign in page, manually applied the change from this PR, opened it in Firefox and tested it there (with the developers tools).
Checklist: