-
-
Notifications
You must be signed in to change notification settings - Fork 719
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) [User->New] Language is displayed twice [OFN-11513] #12655
Conversation
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.
Hi @wandji20 , this doesn't really fix the root cause of the issue, I think we actually need to fix the duplication.
@@ -14,7 +14,7 @@ | |||
= hidden_field_tag "user[spree_role_ids][]", "" | |||
= f.field_container :locale do | |||
= f.label :locale, t(".locale") | |||
= f.select :locale, locale_options, class: "fullwidth" | |||
= f.select :locale, OpenFoodNetwork::I18nConfig.locale_options, class: "fullwidth" |
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 think we are better off fixing the issue here :
def self.available_locales |
The helper function itself isn't wrong (
def locale_options |
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.
Hi @rioug
I am curious as to what the fix here should be.
I simply used local_options as per this comment
Initially, this issue was about removing duplicate English from the dropdown menu list, where @sergioosouzaa removed the souce_local
that is appended to the list of the available locales (later reverted here).
Do I understand correctly that we want to show just one English (which should be thesame for other languages with multiple locals eg fr
, es
) in the dropdown irrespective of the number of en
related locals configured?
eg With this configuration AVAILABLE_LOCALES="en_AU,es,en_US,en_BE,en_CA,es_CO,es_CR,es_US,fr_CH,fr_CA,fr_CM,fr_BE"
, I would expect to see just 3 languages (English, Francais, and Espanol) in the selector?
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.
Fair enough I missed the comment.
I did not quite grasp the problem. From looking at it again, it looks to me to be a translation problem. For instance all the English locales are named "English" that's why we get a duplicates, so I reckon we should be fixing the translations instead.
Before moving forward, let's clarify what they meant,@mkllnk @dacook by this did you mean using non translated locale in the dropdown ? ie having something like en_AU, en_CA, fr etc.. as options ? or did you mean using locale_options
in the helper here :
openfoodnetwork/app/helpers/i18n_helper.rb
Lines 4 to 8 in 0c43dd4
def locale_options | |
OpenFoodNetwork::I18nConfig.available_locales.map do |locale| | |
[t('language_name', locale:), locale] | |
end | |
end |
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 think the issue description was a bit misleading, and the method names are even more confusing.
This is what I meant: dacook@4a9a42e
Wandji could you please try that and test if it solves the problem?
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.
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.
Regarding the multiple "English", (or Espanol, etc), this is not a problem. It is correctly showing en, en_AU, en_UK etc. Only one of these is ever enabled on a system (if we need to enable more, then we can change the label).
The problem we're trying to solve is simply that the en
language always appears, even when it is not enabled.
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 agree with rioug that we should be fixing the translation and still use the
selectable_locales
here
I don't understand, do you mean that we should update i18n_config.rb
(instead of i18n_helper.rb
as I suggested)? As as far as I understand, available_locales
has a different purpose and shouldn't be changed (as per the commnt):
openfoodnetwork/lib/open_food_network/i18n_config.rb
Lines 13 to 21 in 0c43dd4
# Locales that can be selected by users. | |
def self.selectable_locales | |
ENV["AVAILABLE_LOCALES"]&.split(/[\s,]+/) || [] | |
end | |
# All locales that can be accessed by the application, including fallbacks. | |
def self.available_locales | |
(selectable_locales + [default_locale, source_locale]).uniq | |
end |
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.
The problem we're trying to solve is simply that the en language always appears, even when it is not enabled.
For this, I have updated as discussed to use selectable_options which resolves the issue.
As far as fixing translations, I would suggest updating language_name
to match the full language name where possible e.g. In config/locales/en_AU.yml
we could change language_name: "English" to language_name: "English - AUSTRALIA (AU)"
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.
So far we've never needed to have multiple English locales used at the same time (eg in Australia we only enable en_AU; in UK it's only en_UK), so we don't need to update the language_name at this time.
3ee3494
to
9fac209
Compare
Hi @wandji20, Setup of the locales on the serverBefore your PRI could reproduce the issue of duplicate English option: After your PRI checked the following:
ConclusionIt's looking good. I can't find any regressions. Great work! |
What? Why?
Change admin new user local dropdown to use config locale options
What should we test?
Test that admin new user local dropdown will render correct locales
http://localhost:3000/admin/users/new
Release notes
(Fix) [User->New] Language is displayed twice
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
(Fix) [User->New] Language is displayed twice
Dependencies