Skip to content

Add locale selector#5201

Merged
tomhughes merged 1 commit intoopenstreetmap:masterfrom
nertc:issues_1115_add_locale_selector
Jun 14, 2025
Merged

Add locale selector#5201
tomhughes merged 1 commit intoopenstreetmap:masterfrom
nertc:issues_1115_add_locale_selector

Conversation

@nertc
Copy link
Contributor

@nertc nertc commented Sep 12, 2024

This PR addresses "Language switcher on the homepage" issue mentioned in #1115

This PR is mainly based on #5077, #3618 and #4461. As solution using Wikimedia selector added external files, many lines of CSS and JS code, this version takes inspiration from #4461 and creates a reusable language selector using <select> tag.

Language selector button, to be consistent, is not hidden when the user is logged in. Instead, it redirects to the "My Preferences" page, where user can change language preferences. When the user is logged out, clicking language selector opens language selection menu.

The reason for logged in user redirection instead of any complex functionality of preferred language change while clicking language selector, was compatibility with the PR #4461. After both PRs will be merged, there will be one smooth simple UX flow for logged in users. Plus, it makes finding language selection page much easier for new users.

Default view of the language selector:

357859639-0ca3007a-ff80-480b-8b37-1816193dc377

Opened language selector:

image

Mobile view of the selector:

Screenshot_20240912-144716

This PR has many different visual impacts, so to make description shorter instead of directly adding images, I'll add links to them.

LTR screenshots:

Medium resolution menu
Medium resolution menu hover
Small resolution menu

RTL screenshots:

Default view
Medium resolution menu
Small resolution menu

@nertc nertc mentioned this pull request Sep 12, 2024
@AntonKhorev
Copy link
Collaborator

What's the advantage of adding the language icon to the sprite map (three times) as opposed to rendering an <svg> with fill="currentColor"?

@nertc
Copy link
Contributor Author

nertc commented Sep 16, 2024

@AntonKhorev Currently, as I found in the project, for icons we use .icon class, which uses sprite with background-image and background-position properties. Adding icon to the sprite was solely for the accordance with the current state and practices of the project.

@nertc nertc force-pushed the issues_1115_add_locale_selector branch from 64c4f7e to e02866a Compare September 16, 2024 06:11
@nertc
Copy link
Contributor Author

nertc commented Sep 16, 2024

Thank you for the review

@tomhughes I've updated accordingly
@AntonKhorev If it's okay to directly use SVG instead of the .icon class practice of the project, I am all for it. I think it's even better solution and produces cleaner, more reusable code.

@AntonKhorev
Copy link
Collaborator

Adding icon to the sprite was solely for the accordance with the current state and practices of the project.

Currently we use both <svg> and css sprite maps. One problem with sprite maps is that it's more difficult to control the colors of the icons.

app/assets/images/sprite.svg is mainly used for buttons inside Leaflet map views, where we don't entirely control html.

@nertc
Copy link
Contributor Author

nertc commented Sep 16, 2024

@AntonKhorev Okay, thanks. I'll update it to <svg>. It will be more comfortable to reuse and will produce cleaner code.

@nertc nertc force-pushed the issues_1115_add_locale_selector branch from e02866a to 713eaaa Compare September 23, 2024 06:07
@nertc
Copy link
Contributor Author

nertc commented Sep 23, 2024

PR was updated to include SVG in the HTML. I tried several different methods to move SVG from sprite, including adding it to the SvgHelper module and adding separate file for it, but current solution turned out to be the most optimal one (SvgHelper had conflicts with the linter because of long lines and separate file was not sufficiently customizable with Bootstrap classes).

@nertc nertc force-pushed the issues_1115_add_locale_selector branch from 713eaaa to 34d2e25 Compare September 26, 2024 07:52
@nertc nertc force-pushed the issues_1115_add_locale_selector branch from 34d2e25 to f5754b8 Compare October 7, 2024 08:05
@AntonKhorev
Copy link
Collaborator

Looks like the languages you get from Language don't quite match the ones expected by Locale.list. For example, we have pt-BR and pt in Language but pt and pt-PT in translations. As a result you can't select European Portuguese.

@nertc nertc force-pushed the issues_1115_add_locale_selector branch 2 times, most recently from 41cb49e to 80708ff Compare October 17, 2024 08:49
Comment on lines 7 to 12
<% Locale.available
.map { |locale| Language.find_by(:code => locale.to_s) }
.select { |locale| locale }
.sort_by { |locale| locale[:english_name] }
.each do |language| %>
<option class="form-select" value="<%= language.code %>" <%= "selected" if I18n.locale.to_s == language.code %>><%= language.name %></option>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using Language table is probably a bad idea. I tried this:

Suggested change
<% Locale.available
.map { |locale| Language.find_by(:code => locale.to_s) }
.select { |locale| locale }
.sort_by { |locale| locale[:english_name] }
.each do |language| %>
<option class="form-select" value="<%= language.code %>" <%= "selected" if I18n.locale.to_s == language.code %>><%= language.name %></option>
<% Locale.available
.select { |locale| I18n.exists? "shared.language_selector.language", :locale => locale, :fallback => false }
.sort_by { |locale| locale.to_s == "en" ? "English" : t(".language", :locale => locale) }
.each do |locale| %>
<option class="form-select" value="<%= locale.to_s %>" <%= "selected" if I18n.locale.to_s == locale.to_s %>><%= locale.to_s == "en" ? "English" : t(".language", :locale => locale) %></option>

with this in locale file:

  share:
    language_selector:
      language: THIS LANGUAGE NAME

Now translators have to add whatever is the appropriate representation of their language there, and then it will appear in the selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed code can be shortened as Language.where(:code => Locale.available.map {|locale| locale.to_s}). This approach will also cause only one DB query. If there is no other reason for language not to be used, I'll push the code with this approach. Translating language names using i18n and not having one source for them, may cause duplicate translations or inconsistency (because of multiple functionalities using language names).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Translating language names using i18n and not having one source for them

i18n is going to be their one source

may cause duplicate translations or inconsistency (because of multiple functionalities using language names).

We already have two different functionalities using two different sets of languages: i18n and languages of diary entries.

@nertc nertc force-pushed the issues_1115_add_locale_selector branch from 80708ff to 25fd157 Compare October 29, 2024 06:20
@nertc
Copy link
Contributor Author

nertc commented Oct 29, 2024

This PR was updated according to the suggestion of @AntonKhorev.

As described in the CONTRIBUTING.md only updated language config was en:

i18n

If you make a change that involve the locale files (in config/locales) then please only submit changes to the en.yml file. The other files are updated via Translatewiki and should not be included in your pull request.

Because of this, currently only English is shown in the list of language selector. At first, I thought to make a list of all language names in the config file, but when translations will be made, there will be a lot of duplication. The only downside of the current solution is that there will be only those languages shown, which have shared.language_selector.language key.

@AntonKhorev
Copy link
Collaborator

The only downside of the current solution is that there will be only those languages shown, which have shared.language_selector.language key.

Those that have translations that are maintained, so not much of a downside.

I also had a more defensive version of that key with language: THIS LANGUAGE NAME but maybe it's not required. It's possible to document strings on translatewiki like this
https://translatewiki.net/wiki/Osm:Site.copyright.native.native_link/qqq

@nertc
Copy link
Contributor Author

nertc commented Jan 20, 2025

Is there anything that should be fixed in this PR?

@AntonKhorev
Copy link
Collaborator

Depends on how much @gravitystorm is against settings in cookies #4985 (comment).

@nertc nertc force-pushed the issues_1115_add_locale_selector branch 2 times, most recently from b6c464a to d4f0011 Compare February 26, 2025 10:56
@nertc
Copy link
Contributor Author

nertc commented Feb 26, 2025

PR was updated. master was rebased as a base for this PR. Conflicts were resolved. There were some changes related to the header in the master, so logic and HTML of this PR was changed accordingly to fix problems related to locale selector, which were raised after merging. Also, .not-collapsible class was added to the locale selector and, in the future, if any other fixed not-collapsible items will be added to the header menu, adding that class to them will automatically make them not-collapsible.

@nertc nertc requested a review from tomhughes February 27, 2025 07:13
<path d="M20 18h-1.44a.61.61 0 0 1-.4-.12.81.81 0 0 1-.23-.31L17 15h-5l-1 2.54a.77.77 0 0 1-.22.3.59.59 0 0 1-.4.14H9l4.55-11.47h1.89zm-3.53-4.31L14.89 9.5a11.62 11.62 0 0 1-.39-1.24q-.09.37-.19.69l-.19.56-1.58 4.19zm-6.3-1.58a13.43 13.43 0 0 1-2.91-1.41 11.46 11.46 0 0 0 2.81-5.37H12V4H7.31a4 4 0 0 0-.2-.56C6.87 2.79 6.6 2 6.6 2l-1.47.5s.4.89.6 1.5H0v1.33h2.15A11.23 11.23 0 0 0 5 10.7a17.19 17.19 0 0 1-5 2.1q.56.82.87 1.38a23.28 23.28 0 0 0 5.22-2.51 15.64 15.64 0 0 0 3.56 1.77zM3.63 5.33h4.91a8.11 8.11 0 0 1-2.45 4.45 9.11 9.11 0 0 1-2.46-4.45z" />
</svg>
<% unless disabled %>
<select role="button" class="p-0 position-absolute top-0 start-0 w-100 h-100 language-change-trigger text-transparent bg-transparent <%= classes %>">
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like the whole thing about overlaying an SVG on a transparent select to be honest but I don't have a good alternative either.

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 tried several different methods to solve it, but still for me the best one is the current solution. If you prefer any of them, I can change code accordingly:

  1. Showing dropdown next to the icon.
    Pros:

    • Both Icon and selected language are shown
    • Doesn't use absolute position and dropdown hiding

    Cons:

    • Takes more space in the menu
    • Clicking icon doesn't automatically open dropdown (it is only focused), therefore, needs more JS to add opening functionality

    image

  2. Adding <svg> or <img> tag directly in the <select><option> tag and show icon as selected.
    Cons:

    • Browser blocks such cases and doesn't render <svg> and <img> in the <option>

    image

  3. Use Bootstrap dropdown.
    Pros:

    • Icon and selection button are shown
    • Icon and selection button are same item
    • Uses Bootstrap

    Cons:

    • As there are many languages, dropdown gets too long and exceeds website height
    • Needs some custom CSS to achieve good sizing and consistent visuals
    • In terms of the responsive design, <select> tag has much better and familiar UX than Bootstrap Dropdown

    image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I haven't changed anything related to this comment. If you prefer any of these or other solutions, I'll update PR accordingly.

@nertc nertc force-pushed the issues_1115_add_locale_selector branch 2 times, most recently from a0478ea to 8a3e211 Compare March 12, 2025 18:13
@nertc
Copy link
Contributor Author

nertc commented Mar 12, 2025

PR was updated. master was rebased as a base for this PR.
@tomhughes Code was changed according to comments.

@nertc nertc force-pushed the issues_1115_add_locale_selector branch from 8a3e211 to 39a0abf Compare April 7, 2025 06:49
@nertc
Copy link
Contributor Author

nertc commented Apr 7, 2025

Thanks for rerunning tests.
PR was updated. master was merged to avoid previous test errors.

@nertc nertc force-pushed the issues_1115_add_locale_selector branch from 22a9c4e to 4845308 Compare June 11, 2025 09:46
@nertc
Copy link
Contributor Author

nertc commented Jun 11, 2025

PR was updated. master was merged.

@AntonKhorev Thank you for your PR. Locale selector was changed accordingly.

@tomhughes
Copy link
Member

I think this is fine as a starting point and we should merge it - we can always look to improve things further later.

Thanks for all the work on it.

@tomhughes tomhughes merged commit c5fe67e into openstreetmap:master Jun 14, 2025
16 checks passed
@tomhughes tomhughes mentioned this pull request Jun 14, 2025
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.

4 participants