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 i18n helpers in core #9281

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add i18n helpers in core #9281

wants to merge 5 commits into from

Conversation

yohanboniface
Copy link
Member

I need a module way of importing the translate function in uMap. So either I modernize Leaflet.i18n, either I contribute here, which may a better move. So let's try something here :)

Trying to summarize what I think we agreed on (cf #9092):

  • the i18n helpers are added into Leaflet core
  • the translations strings are not added into Leaflet core
  • we use the original strings as keys
  • naming: translate seems to be the better consensus for the main helper (vs _, i18n…)

Two points that still needs discussion and decision:

  • should we deal with namespace for collisions between plugins in translatable strings
  • how to collect the translatable strings

About the first point, I'm not convinced this is more than a edge case. Leaflet.i18n does not deal with this, and has been used for years without nobody asking for it. I guess adding the feature into Leaflet will give it more visibility and usage, so this may arrise, but let's focus on actual need.
Keeping things simple is often a better choice in the long term, so I've made this choice for this PR.
Given that the order we call registerLocale is important, one can still make sure the preferred translations are loaded last to be sure they override any possible collision.

About the second point, Leaflet.i18n exposes a script to collect the translatable strings among the code base and put it in a json file, that one can then easily push to Transifex or such service. This comes with some conventions (eg. a json file name lang.json), so maybe in a first step it would be better to keep this outside of Leaflet, and maybe rename Leaflet.i18n to something like leaflet-collect-i18n, so people wanting this feature would integrate it in their workflow.

Translation is a whole world per se, and the API we need to elect is not just about how to make a string translatable in the code. We need to keep things simple even for people using many plugins, people wanting to mix leaflet plugins with their own Javascript code that includes other translatable strings, people wanting to add more languages for their site using Leaflet and plugins, and needing to coordinate all those string in a Transifex like service, etc.

Also, one thing to keep in mind: translate is not lazy, so it cannot be used for example for options in a class definition, or any situation where the function will be executed at load time (unless the registerLocale has been used before of course, which is not the case for Leaflet core but can be for plugins).

When (if!) we are ready to merge, I'll add a tutorial for a better overview on how to set up translations.

As an exercise and a demonstrator, I've used translate internally in this PR for Leaflet strings ('zoom in'…), but I'm not sure it actually makes sense if we do not have the translations alongside. And certainly at this point we should only make sure it's easy and documented to override the internal strings through options. Or maybe it makes sense if those translations could be in a separate package (ping @nfreear) ?

Thanks for your feedback! :)

Copy link
Member

@Falke-Design Falke-Design left a comment

Choose a reason for hiding this comment

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

Very nice PR, thank you!

should we deal with namespace for collisions between plugins in translatable strings
...
I'm not convinced this is more than a edge case. [...] Keeping things simple is often a better choice in the long term

I agree with you

how to collect the translatable strings

I would like to have a repository in the Leaflet Github Space where all the translations for Leaflet Core are available. With that we make sure that the Leaflet Core Team has always access to the repo to maintain it.
We could provide each language as own javascript file and provide a module to import all or only certain languages. If someone need them in a different format, it can be easily copied from the javascript file.
I don't know if we should have translation of plugins somewhere (or in the same repo) stored. The plugin maintainers can always provide their own translation files but what if the plugin get unmaintained? It would sad if everyone needs to translate it always by him self again instead of having one central place.

export function addLocalesFR () { // we need a better name ... :D
		const fr = {
			'Simple phrase to translate': 'Une simple phrase à traduire',
			'A phrase with a {variable} to translate': 'Une phrase à traduire avec une {variable}',
			'A phrase with empty translation': ''
		};
		registerLocale('fr', fr);
}

Leaflet.i18n exposes a script to collect the translatable strings among the code base

Yes let us keep this outside of Leaflet.

Also, one thing to keep in mind: translate is not lazy, so it cannot be used for example for options in a class definition

good point. Does it make sense to remove all text options? Because if someone wants another text, he can simply add a translation for it.

As an exercise and a demonstrator, I've used translate internally in this PR for Leaflet strings ('zoom in'…), but I'm not sure it actually makes sense if we do not have the translations alongside.

I think we should do this, because if we support translation every text in Leaflet should be translate able over the i18n functions.

src/core/I18n.js Outdated Show resolved Hide resolved
// a bad translation should not break the app
string = Util.template(string, data);
} catch (err) {
console.error(err);
Copy link
Member

Choose a reason for hiding this comment

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

Just to think about it: Should we add a prefix to the error, so that it can be easily recognized as a translation error?

grafik

src/core/I18n.js Outdated Show resolved Hide resolved
src/core/I18n.js Outdated Show resolved Hide resolved
src/core/I18n.js Outdated Show resolved Hide resolved

// @option zoomOutText: String = '<span aria-hidden="true">&#x2212;</span>'
// The text set on the 'zoom out' button.
zoomOutText: '<span aria-hidden="true">&#x2212;</span>',

// @option zoomOutTitle: String = 'Zoom out'
// The title set on the 'zoom out' button.
zoomOutTitle: 'Zoom out'
zoomOutTitle: ''
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove the options completely?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me! But then letting the doc string there or is there another way to document a non declared option ?

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 the doc string would be the correct way. But I was thinking about removing the option and the doc sting completely because there is no need anymore for them.

Does it make sense to remove all text options? Because if someone wants another text, he can simply add a translation for it.

yohanboniface and others added 3 commits February 27, 2024 21:29
Co-authored-by: Florian Bischof <design.falke@gmail.com>
Co-authored-by: Florian Bischof <design.falke@gmail.com>
This may let plugins authors and users to interact with the
registered translations.
@Falke-Design
Copy link
Member

@jonkoops @mourner @IvanSanchez I would be nice to have your input too but if you have no time to review, please give your OK that we implement translation in Leaflet core.

@jonkoops
Copy link
Collaborator

I'll try to get a review in for this, can't tell exactly when that will be.

yohanboniface pushed a commit to umap-project/umap that referenced this pull request Mar 4, 2024
yohanboniface added a commit to umap-project/Leaflet.i18n that referenced this pull request Mar 22, 2024
We want to deal with possible integration of i18n into Leaflet,
where name of the function will change.

cf Leaflet/Leaflet#9281
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.

None yet

3 participants