Closed brunoocasali closed 3 years ago
Thanks for this fix!
I find that it's mostly working. But there is one test failure. One of the tests uses the search bar and ends up on a page with an invalid locale.
To avoid this, I think it's best to not include the hidden_field_tag
(the one that sets the locale
query string for the search bar results page) if the locale
parameter is unset.
I have a commit to solve this, let me know if this fix looks good to you, or if you have any issues with this fix: https://github.com/DeeDeeG/refugerestrooms/commit/dff451d6687991a20673414a4773f2c8e9e37c37
I can probably push that commit to your branch, or you can commit a fix yourself or merge this commit, but I didn't want to do so without asking first.
Best Regards,
- DeeDeeG
@DeeDeeG you can delete this branch if you want!
I don't ran the tests before (my bad - sorry!), actually I was looking for the Travis CI result, but it stucks on some kind of build, and then I wait for your response ;)
I think your fix is pretty nice, and we can use it, an alternative to it is just add the default language to that hidden field:
= hidden_field_tag :locale, params[:locale] || I18n.default_locale
, is less verbose than the first option but equally effective (all the tests passes here).
I will think about which solution to use tomorrow as it is late in my time zone. But then this will be ready to go.
With that, all the hard-coded links are gone. The locale switcher should be ready once this fix lands on the locale switcher branch. 🎉
Hi again.
After testing, I've decided it's best to not set a locale in the search bar, if a locale hasn't been set yet.
The problem with falling back to I18n.default_locale
(en
), is that it doesn't match the logic from here:
(Visitors to the site may have a locale auto-determined to be something other than our default: English. In which case the search bar should not send them to the English site.)
And writing out all that locale-auto-setting logic here would be code duplication and would be rather verbose. Either we can refactor all that out into a helper (maybe? I've never refactored Ruby/Rails code in that way so I don't know if it makes sense to try it here...) or simply have the search bar not set a locale at all if it's not already set.
The "don't set a locale" option makes the most sense to me personally. Though I admit the helper option might be okay. I like simple code, too, and "no helper" is simpler.
Describing the problem practically: A user could visit the site at https://refugerestrooms.org
(no ?locale=
query string) and have their ideal locale automatically determined to be, let's say fr
. Then they click the search bar, which sends them to the English site. They would have expected to stay on fr
.
I can amend this Pull Request to the solution I linked to before (https://github.com/DeeDeeG/refugerestrooms/commit/dff451d6687991a20673414a4773f2c8e9e37c37) or if you have another idea I'd be open to discuss it as always.
Best Regards,
- DeeDeeG
@DeeDeeG after many interactions in this code I think this is the best of it! If you have other use cases please tell me ;)
More code to review here, I'm thinking it over. Looks like a working design, no bugs, great. 👍
On my mind:
This one is more than I can get to in one day, but hopefully soon.
@DeeDeeG the ideia behind the LocaleService
is just to provide a way not just for reuse but mainly for testability.
If I keep all the logic inside of the application_controller, then I should deal with rails complexities, and of course the possibilities of reuse will be lower.
There are something I can do to help you to send this whole feature to production? (btw I joined the slack channel, we could talk there)
CC: @DeeDeeG I've pointed this PR to your branch because I've no access to create commits there 😝