Preserve the url hash when changing locales. #2964
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jfly
force-pushed
the
preserve-hash-when-changing-locale
branch
from
e116f80 to
afd76ad
Compare
viroulep
approved these changes
Jun 15, 2018
| var csrfToken = $('meta[name=csrf-token]').attr('content'); | ||
| var csrfParam = $('meta[name=csrf-param]').attr('content'); | ||
| var form = $('<form method="post" action="' + href + '"></form>'); | ||
| var metadataInput = '<input name="_method" value="' + method + '" type="hidden" />'; |
There was a problem hiding this comment.
Can't we simply use "patch" here instead of a separate variable?
There was a problem hiding this comment.
Good idea! You are seeing evidence of my copy paste-ing =)
| var method = "patch"; | ||
| var csrfToken = $('meta[name=csrf-token]').attr('content'); | ||
| var csrfParam = $('meta[name=csrf-param]').attr('content'); | ||
| var form = $('<form method="post" action="' + href + '"></form>'); |
There was a problem hiding this comment.
href is used only here, so you can maybe just use this.href here and drop the variable.
There was a problem hiding this comment.
Good idea! You are seeing evidence of my copy paste-ing =)
To implement this, I had to write a custom javascript handler to handle switching locales. Previously, this was handled for us via Rails's unobtrusive javascript, but I could not find any way to add in additional url parameters to the PATCH request the unobtrusive javascript was generating for us. There's actually an open PR from 2013 to implement support for this here: rails/jquery-ujs#307. I also rewrote the set_locale_spec to enable javascript and be more like what I believe a feature spec should be. I actually don't understand how the spec was even passing before, I would expect it to fail because it was sending a GET request, and we don't have a handler for GET request to update the locale. Maybe rspec/capybara/poltergiest has baked in support for unobtrusive javascript so that the right thing will happen even if you don't have javascript enabled?? That sounds crazy, but it's the only thing I can think of. Regardless of how this was working before, I do believe that the new specs are better. This fixes thewca#2962.
jfly
force-pushed
the
preserve-hash-when-changing-locale
branch
from
afd76ad to
4d913ad
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To implement this, I had to write a custom javascript handler to handle
switching locales. Previously, this was handled for us via Rails's
unobtrusive javascript, but I could not find any way to add in
additional url parameters to the PATCH request the unobtrusive
javascript was generating for us. There's actually an open PR from 2013
to implement support for this here: rails/jquery-ujs#307.
I also rewrote the set_locale_spec to enable javascript and be more like
what I believe a feature spec should be. I actually don't understand how
the spec was even passing before, I would expect it to fail because it
was sending a GET request, and we don't have a handler for GET request
to update the locale. Maybe rspec/capybara/poltergiest has baked in
support for unobtrusive javascript so that the right thing will happen
even if you don't have javascript enabled?? That sounds crazy, but it's
the only thing I can think of. Regardless of how this was working
before, I do believe that the new specs are better.
This fixes #2962.