Open nertc opened 2 months ago
What's the advantage of adding the language icon to the sprite map (three times) as opposed to rendering an <svg>
with fill="currentColor"
?
@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.
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.
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.
@AntonKhorev Okay, thanks. I'll update it to <svg>
. It will be more comfortable to reuse and will produce cleaner code.
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).
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.
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 theen.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.
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
This PR addresses "Language switcher on the homepage" issue mentioned in https://github.com/openstreetmap/openstreetmap-website/issues/1115
This PR is mainly based on https://github.com/openstreetmap/openstreetmap-website/pull/5077, https://github.com/openstreetmap/openstreetmap-website/pull/3618 and https://github.com/openstreetmap/openstreetmap-website/pull/4461. As solution using Wikimedia selector added external files, many lines of CSS and JS code, this version takes inspiration from https://github.com/openstreetmap/openstreetmap-website/pull/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 https://github.com/openstreetmap/openstreetmap-website/pull/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:
Opened language selector:
Mobile view of the selector:
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