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 basic i18n support #941

Merged
merged 6 commits into from Jan 18, 2024
Merged

Add basic i18n support #941

merged 6 commits into from Jan 18, 2024

Conversation

jesperengstrom
Copy link
Contributor

@jesperengstrom jesperengstrom commented Jan 12, 2024

Description

Tickets involved

LXL-4362

Summary of changes

A modest proposal on how we could approach the i18n issue. Svelte has no official solution, and after looking at a number of libs, many seem too large, hacky or abandoned.

I then found this example that felt like a good approach and decided to try it out. It's basically a function that loads the needed translations and returns a translator function from the layout. It supports nested translation files and basic string interpolation (hello {name} how are you) and can easily be extended to support our exact needs (unknown to me at this point...)

Some notes:

  • Translation files are .js at the moment. This allows us to typecheck the en using the sv one for example, keeping them fully synced. But we could just as well use json files.
  • No cookie solution atm. Locales strictly follows the lang param (or lack of). If introduced, we'll have to discuss what should have precedence if both exist.
  • Found no way to globally change the url base path (/en/ etc) at runtime, which is needed for correct links to be rendered. Current solution exposes a base variable that needs to be prepended to all links, which is a bit tedious...

@jesperengstrom jesperengstrom marked this pull request as ready for review January 15, 2024 11:12
Copy link
Contributor

@olovy olovy left a comment

Choose a reason for hiding this comment

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

LGTM!

I've come to prefer the gettext style of using the actual message as key. But I have no strong preference.

As you say this could easily be extended with plural forms etc. Agree on the link rewriting being tedious.

A nice starting point.

@johanbissemattsson
Copy link
Contributor

johanbissemattsson commented Jan 15, 2024

I agree with it being a good starting point! I can do a more formal review this Wednesday (but I am also fine with merging it).

A quick idea regarding the issue with repetitive base url paths: could a <base> element in the root +layout.svelte be used to specify the base url path for all relative URLs on the site?

I have no real preference regarding actual message style but it can be good to further discuss it and how we can achieve pluralization.

@jesperengstrom
Copy link
Contributor Author

A quick idea regarding the issue with repetitive base url paths: could a <base> element in the root +layout.svelte be used to specify the base url path for all relative URLs on the site?

Yay, that worked! I tried that before, but missed that you then can't use root-relative paths (/search)

@jesperengstrom
Copy link
Contributor Author

jesperengstrom commented Jan 18, 2024

Merging this now, continued discussion has been mostly about the base thing - i.e what's least annoying/error prone; <base> element disallowing root-relative (/) links, a {base} variable or a getLink function. Guess we'll just try it out and see.

@jesperengstrom jesperengstrom merged commit 1a6a6a3 into develop Jan 18, 2024
@samuelstroschein
Copy link

@jesperengstrom

Why did Paraglide JS seem "too large"? We need to improve our communication :) cc @LorisSigrist

@jesperengstrom
Copy link
Contributor Author

Hi @samuelstroschein, sorry for the sloppy comment (edited now). What I meant was that in our particular case and right now any lib probably has more functionality than we need. Paraglide seems great btw and something we'll probably consider again at some point :)

@jesperengstrom jesperengstrom deleted the feature/I18n-lab branch January 24, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants