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

Add locale chooser to admin #2559

Merged
merged 9 commits into from
Mar 29, 2018
Merged

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Feb 6, 2018

I want to add a locale selector to the admin.

It's currently too hard to change the locale of the admin. My hope is that by making it easier we will have more users switch to a non-english locale and then through that hopefully get more help keeping them up to date.

I want to implement this in Solidus backend rather than solidus_i18n. This lets us avoid the need for deface, and makes it more clear that i18n is a concern of Solidus itself.

It would be nice if we could recommended that all new users include solidus_i18n, and listed the gem inside our README's "Getting started" section. To do this we would probably want it to only provide YAML files and there's been some work towards that.

Todo

  • Have the selector actually set the locale
  • Currently this uses I18n.available_locales, we should probably restrict it to locales which have at least some translations under the spree namespace.
  • Hide selector if there is only one available locale.

@jhawthorn jhawthorn added the WIP label Feb 6, 2018
kennyadsl
kennyadsl previously approved these changes Feb 8, 2018
@kennyadsl kennyadsl dismissed their stale review February 8, 2018 11:45

didn't noticed that it was WIP

@solidusio solidusio deleted a comment from houndci-bot Mar 6, 2018
@solidusio solidusio deleted a comment from houndci-bot Mar 6, 2018
@jhawthorn jhawthorn force-pushed the locale_chooser branch 3 times, most recently from 2800684 to 90836f6 Compare March 7, 2018 00:32
@solidusio solidusio deleted a comment from houndci-bot Mar 7, 2018
@solidusio solidusio deleted a comment from houndci-bot Mar 7, 2018
@solidusio solidusio deleted a comment from houndci-bot Mar 7, 2018
@jhawthorn jhawthorn force-pushed the locale_chooser branch 2 times, most recently from 7314080 to 6c6165f Compare March 7, 2018 21:06
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a couple of notes, thanks!

@@ -0,0 +1,15 @@
<div class="admin-locale-selection">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should put this div into the if available_locales.size > 1? I think there's no reason to keep the html if empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@@ -5,9 +5,15 @@

RSpec.describe "i18n" do
before do
# This reload avoids an issue with I18n.available_locales being cached
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have ideas about why this is needed only in this specs and not in other files that uses I18n?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the other files don't need a new locale to be autodetected. We should keep an eye out for new failures related to this, though

<%=
options_for_select(
available_locales.map do |locale|
[I18n.t('spree.i18n.this_file_language', locale: locale, default: locale.to_s, fallback: false), locale]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using some more explicit string to mark locales available, like active: true?
I know it's not a proper translation but how can a user easily understand that this key is what will make the translation file available?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good for now and works with existing locales w/o any further changes.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great. Thanks!

<%=
options_for_select(
available_locales.map do |locale|
[I18n.t('spree.i18n.this_file_language', locale: locale, default: locale.to_s, fallback: false), locale]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good for now and works with existing locales w/o any further changes.

jhawthorn and others added 9 commits March 28, 2018 15:03
This filters I18n.available_locales to return only those with
translations for Solidus.
This prevents using locales which have some, but not most i18n
translations defined. This fixes an issue when including
solidus_auth_devise, but not solidus_i18n. Previously this method would
misidentify the few non-en translations in solidus_i18n as forming a
valid locale to switch to.
@jhawthorn jhawthorn merged commit a779d17 into solidusio:master Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants