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

feat: Add placeholder function for translation/localization/i18n to Leaflet #9092

Open
3 tasks done
nfreear opened this issue Sep 15, 2023 · 7 comments
Open
3 tasks done
Labels
accessibility Anything related to ensuring no barriers exist that prevent interactions or information access feature
Milestone

Comments

@nfreear
Copy link
Contributor

nfreear commented Sep 15, 2023

Checklist

  • I've searched through the plugins to make sure this feature isn't already available, or think it shouldn't require a plugin.
  • I've searched through the current issues to make sure this feature hasn't been requested already.
  • I agree to follow the Code of Conduct that this project adheres to.

Motivation

Currently, core Leaflet and Leaflet plugins are not translated from English. This is not a big problem for visual users, as most controls like the "Zoom in" button rely on icons. However, for non-visual and low vision users, for example those relying on screen readers it is a significant barrier, when maps are implemented on non-English websites (accessibility).

You may argue that implementors should set the zoomInTitle option for the control. However, that is not exposed through the main map options, making it fiddly and inconsistent.

And, there are a number of accessibility-related open bugs and pull requests that add new text strings to Leaflet. For example:

Suggested solution

I propose adding a placeholder or "dummy" for a translation function to core Leaflet. This would be a very small "pass-through" function that simply returns the string that is input as the first parameter.

That is, as an arrow function or non-arrow function:

L._ = L._ || ((str) => str);
L._ = L._ || function translatePlaceholder (string) { return string; }

Or, in a form that handles a second data parameter:

L._ = function translate (string, data) {
    try {
        // Do not fail if some data is missing - a bad translation should not break the app.
        string = L.Util.template(string, data);
    }
    catch (err) { } /*pass*/

    return string;
};

This placeholder would be called wherever needed in core Leaflet - Control.Zoom.js:

  options: {
    
    zoomInTitle: L._('Zoom in'),
    zoomOutTitle: L._('Zoom out'),
    
  }

And, it can be adopted by third-party plugins @brunob/leaflet.fullscreen - ...js:

  options: {
    
    title: L._('Full Screen'),
    titleCancel: L._('Exit Full Screen'),
    
  }

Re-implement

Finally, anyone wishing to implement a map on a Non-English website would use a third-party internationalization plugin, for example, Leaflet.i18n, which re-implements the L._() function.

Benefits of this approach:

  1. A standardized, consistent, documented function is available to translate EVERY string in Leaflet for core and third-party plugins.

  2. A lightweight approach. No need to implement all the internationalization functionality within Leaflet. No need to add language packs to core Leaflet. Instead, rely on an external plugin and language packs/locale files as needed.


Steps

  1. Implement L._() function in core Leaflet,
  2. Adopt the L._() function throughout Leaflet,
  3. Write unit-tests for use of the L._() function.
  4. Document the L._() function in the API docs,
  5. Document the L._() function in the Plugin Guide,
  6. Identify pathfinder third-party plugins to adopt the L._() function, for example a fullscreen plugin.
  7. Create a repo. containing reference/starter language packs/locale files, for some widely used languages.
  8. Blog it!

This proposal is inspired by @yohanboniface's "Leaflet.i18n" plugin, PR #1232, and GNU Gettext.

I welcome feedback on this proposal!

Thanks.


Alternatives considered

The Leaflet.a11y plugin is a short-term, non-optimal alternative. The solution above is much better long-term solution.

See the open PR: #9087, to list "Leaflet.a11y" in the plugin list!

@Falke-Design
Copy link
Member

I also think it is time to add a translation function in Leaflet. Like @yohanboniface predicted in #1232 only a few plugins are translated. We can help to change this with only a few bytes.

But instead of implementing a placeholder, I would implement the logic for the translation too. Pretty similar to Leaflet.i18n, only with a few changes:

  • Make it possible to use nested paths. With that every plugin can have his name as prefix and nothing is overwritten: L.i18n('Leaflet.control.zoomOut')
  • With Leaflet V2 we will move away from the global L, so the function will be imported directly. This means we need a better name then _ or i18n. import i18n from 'leaflet';
  • ...

But we should not provide any translation files. This should be in a different repo like you suggested.

@Falke-Design Falke-Design added accessibility Anything related to ensuring no barriers exist that prevent interactions or information access and removed needs triage Triage pending labels Sep 15, 2023
@nfreear
Copy link
Contributor Author

nfreear commented Sep 21, 2023

Hi @Falke-Design,

Thanks for the response. Interesting thoughts. A few points:

But instead of implementing a placeholder, I would implement the logic for the translation too.

History including #1232 suggests concern to keep core Leaflet small and simpler. Also, when a Leaflet map is just one component in a site based on e.g. WordPress or Vue.js, the implementor probably wants localization handled by that system, not by Leaflet on its own. We need to get a consensus on this.

  • Make it possible to use nested paths. With that every plugin can have his name as prefix and nothing is overwritten: L.i18n('Leaflet.control.zoomOut')

Libraries patterned after GNU Gettext tend to use _("The untranslated string") - This has the benefit that if the string is missing from the language pack, or something is broken, you should get something vaguely useful out. Commandline tools can be used to extract the untranslated text from source files in lots of languages - C, Python, PHP ... Javascript.

And, for the string "Zoom out", do I care that it's for core Leaflet, or another plugin? Surely, I just care that it's a good translation?

... This means we need a better name then _ or i18n. import i18n from 'leaflet';

The function name _ is not random. It's borrowed from GNU Gettext. That being said, I don't mind what the name is within reason, as long it's fairly short. Maybe translate("My string")?

We should probably look at what others are doing in this space:

Thanks

Nick

@yohanboniface
Copy link
Member

A few thoughts:

  • I agree that including the L._ in the core would help a lot the Leaflet ecosystem to finally have translations
  • I agree that it's better to keep the actual translation function and the related string out of the core, to keep in small as possible, and have translation has an "opt-in" function, that would be added by including a plugin, just like for example Path.Drag.js is included when someone wants this feature
  • Having namespaced keys instead of actual strings would imho make the translation maintenance harder: this adds a step to understand what the key is about and can make the process of translating longer (in particular when using third party sides like transifex and such)
  • the Leaflet.i18n translation function is anyway very simple (does not deal with plurals nor any sort of derivative/context), and I think it's fine for the purpose (it's sufficient for uMap, which is translated in around 50 languages), but those limitations should be clear from the beginning and maybe should be documented
  • another way to keep Leaflet core light could be to "just" make Leaflet.i18n more "official" and documented, and having more options to override Leaflet internal labels

@Falke-Design
Copy link
Member

I agree that it's better to keep the actual translation function and the related string out of the core, to keep in small as possible, and have translation has an "opt-in" function, that would be added by including a plugin, just like for example Path.Drag.js is included when someone wants this feature

In my eyes we should go away of thinking translation and accesibillity is opt-in. I think many things can be a plugin but translation and accesibillity should be always available and then it should be in the core. Doesn't matter if Leaflet is only showing the position of a company or is used for a whole application. This few bytes are worth it. (329 bytes)

grafik

But if we provide it as external plugin, then it should be possible for the plugins to use L.i18n("Hello World") without having the plugin as dependency. This would be simply L.i18n = function(str)=>return str;

Having namespaced keys instead of actual strings would imho make the translation maintenance harder

That is true but there would be maybe some problems when the same text has multiple translations. We can prevent this when it is possible to add a preferred translation-set L.i18n("Hello World", "leaflet-geoman")

the Leaflet.i18n translation function is anyway very simple (does not deal with plurals nor any sort of derivative/context), and I think it's fine for the purpose (it's sufficient for uMap, which is translated in around 50 languages), but those limitations should be clear from the beginning and maybe should be documented

Fine by me. A solution could be that we make it possible to pass a function for the translation and then it can be determined by the data which text should be returned. On the other hand, the user can do this internally in his code with some if-else

@jonkoops
Copy link
Collaborator

jonkoops commented Nov 6, 2023

I am also a proponent of implementing localization in Leaflet itself, this has been a request many times before and can be a valuable addition for both us and especially authors of plugins to standardize.

So let's talk implementation.

Using the 'gettext' style of passing in the English source has my preference for a couple of reasons. Firstly, it is instantly clear what the text is supposed to be, no need for additional lookups. Secondly, if the text changes, so does the 'key', forcing translations to be updated. It also means we can simply use the string as-is for Leaflet without localization.

The function that does the translation should be short and to the point. I also am not a proponent of using yet another single special character function (such as _ or $), it should be a named function that is easy to understand. I also would leave things like namespacing for plugins out of scope, we can always add this later if the demand for it exists.

Perhaps we could use the letter t as a function name? It's already something that I am personally familiar with from i18next. Either way, this is a bikeshed, we just need to agree on something.

Taking @nfreear's original example, I would imagine something like this for a high-level API:

import { t } from "leaflet";

const options = {
  zoomInTitle: t("Zoom in"),
  zoomOutTitle: t("Zoom out"),
};

I also agree with the idea to provide interpolation based on the existing template() syntax. Leaflet users and plugin authors are already familiar with this syntax, so it would be trivial to adopt:

import { t } from "leaflet";

// => "Hello, Jane Doe." 
const message = t("Hello, {firstName} {lastName}.", {
  firstName: "Jane",
  lastName: "Doe",
});

So let's agree on this first, so we can move on to the API design of the part that is responsible for providing the the localizations to Leaflet itself. And the mechanism which Leaflet will use to detect the locale, or allow it to be set externally.

@Leaflet/core @Leaflet/leaflet-maintainers can we bikeshed the above and agree on this part of the API?

@Falke-Design
Copy link
Member

Using the 'gettext' style of passing in the English source has my preference for a couple of reasons. Firstly, it is instantly clear what the text is supposed to be, no need for additional lookups. Secondly, if the text changes, so does the 'key', forcing translations to be updated.

But on the down side this has the problem (especially for plugins) that the same key is used in different projects and can be translated different. Which translation will win and be displayed?

It also means we can simply use the string as-is for Leaflet without localization.

That is true and a big benefit.

Perhaps we could use the letter t as a function name?

I personally don't like this single characters. It is a function so let us use a name which can be directly understand by everyone for example translate() or i18n. It will be much easier for new contributers to understand the code.
Also we don't need to have the bundle size in mind, because it will be minified anyway. The only thing against a longer function name is, that some code lines will be getting longer but we get more readable code. And the developers can always import it with another name import { translate as t } from "leaflet"; too.

For the problem with the same key: the same key is used in different projects and can be translated different. Which translation will win and be displayed?

I have two solutions for this:

  1. The developers can specify from which translation-set it will be translated. Like L.i18n("Hello World", "leaflet-geoman")
  2. Each plugin extends the translate function which causes that the plugin translation will be used first:
import { translate as translateLeaflet, getLocale} from "leaflet";

const pluginTranslations = {
	"en": {
		"Zoom in": "Zoom in",
		"Zoom out": "Zoom out",
	},
	"de": {
		"Zoom in": "Hinein zoomen",
		"Zoom out": "Heraus zoomen",
	},
};

export function translate(let key) {
	if(pluginTranslations[getLocale()] && pluginTranslations[getLocale()][key]){
		return pluginTranslations[getLocale()][key];
	}
	return translateLeaflet(key);
}

@jonkoops
Copy link
Collaborator

But on the down side this has the problem (especially for plugins) that the same key is used in different projects and can be translated different. Which translation will win and be displayed?

Yeah, that is always a concern. But I haven't really ran into this issue in practice. Personally, I think the benefit of having the source there, without mapping values, outweighs this.

I personally don't like this single characters. It is a function so let us use a name which can be directly understand by everyone for example translate() or i18n.

And the developers can always import it with another name import { translate as t } from "leaflet"; too.

I agree with all of this. Shall we settle on translate()?

As for the name-spacing for translations, I would have to go with option 1. Perhaps we could use a Symbol as a first argument to the translate function?

import { translate } from "leaflet";

const PLUGIN_NAMESPACE = Symbol("pluginName");
const message = translate(PLUGIN_NAMESPACE, "Hello, World");

Perhaps a simple string would suffice, but it would force a single and unique value. Alternatively to option 2, we could provide a function that is a 'factory' for name-spaced translation functions. For example:

import { createTranslateFn } from "leaflet";

const PLUGIN_NAMESPACE = Symbol("pluginName");
const translate = createTranslateFn(PLUGIN_NAMESPACE);
const message = translate("Hello, World");

This would ensure all translations are still name-spaced, without sacrificing too much convenience for plugin authors, specifically having to repeat the namespace for every translation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Anything related to ensuring no barriers exist that prevent interactions or information access feature
Projects
None yet
Development

No branches or pull requests

5 participants