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

chore: add localization to UI and components #153

Merged
merged 21 commits into from
Aug 28, 2024
Merged

Conversation

GregoMac1
Copy link
Collaborator

@GregoMac1 GregoMac1 commented Aug 12, 2024

Closes #150

@GregoMac1 GregoMac1 self-assigned this Aug 12, 2024
@fmaclen fmaclen changed the title feat: Add localization to UI and components chore: add localization to UI and components Aug 15, 2024
@fmaclen
Copy link
Owner

fmaclen commented Aug 15, 2024

Couple of edits:

  • Updated PR title to chore: prefix so it doesn't trigger a release
  • I removed Closes #151, we should do that in a separate PR

@GregoMac1
Copy link
Collaborator Author

@fmaclen I'm still working on this, I still haven't tried all the possible cases to see is every text is showing correclty.

Also, I've just merged main and I can see that there are new texts that I have to localize.

However, I'd like to ask you to please review it to see if the current approach is correct or if there's anything that needs to be changed.

PS: About the failing test, I don't exactly know why it fails. When I run npm run test on my computer, all tests finish successfully. Could it be something about the screenshots' size?

Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

I'm not sure if you are still working on this but I left you some note changes.

In my experience with large projects that have a lot of localized strings grouping keys by "section" doesn't work very as the project grows.

Mainly because you end up with generic keys such as name, save, delete, server, etc that need to be used in other "sections", which if you are not super diligent about keeping things organized you quickly run into duplicates.

I think your grouping is okay for now, but we will probably start moving most of those keys up to the parent en tree.

src/lib/i18n.ts Outdated Show resolved Hide resolved
src/lib/i18n.ts Outdated Show resolved Hide resolved
src/lib/i18n.ts Outdated Show resolved Hide resolved
src/lib/i18n.ts Outdated Show resolved Hide resolved
src/lib/i18n.ts Outdated Show resolved Hide resolved
@GregoMac1
Copy link
Collaborator Author

Thanks for the feedback! I'll apply the corrections.

Also, I was wondering on how to translate this, as it has text interpolated with html tags. Do you have any suggestion?

@fmaclen
Copy link
Owner

fmaclen commented Aug 23, 2024

I was wondering on how to translate this

Yeah, these cases are tricky because we would need to interpolate components in the middle of a string.

I think the realistic solution to this issue is to re-write the sentences in such a way so the labels in the components can stand alone.

For example:

<p>
  {{ Change your server settings to allow connections }}
  <Badge capitalize={false}>OLLAMA_ORIGINS={ollamaURL.origin}</Badge>
  <a>{{ See docs }}</a>.
  {{ Also check no browser extensions are blocking the connection. }}
</p>

And in this example we can keep the <code> tags in the translated strings:

<p>
  {{ If you want to connect to an Ollama server that is not available on <code class="code">localhost</code> or <code class="code">127.0.0.1</code> you can try: }}
  <a>{{ Creating a tunnel }}</a>
  <a>{{ Allowing mixed content }}</a>
</p>

It sounds a bit more "robotic" but I think it's fine for now.

@fmaclen fmaclen marked this pull request as ready for review August 25, 2024 19:04
Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

  • Minor change in the sectionPage naming convention.
  • We are missing the localized sentences in the Settings page, I left you some suggestions in this comment.
  • We are also missing some dialog confirmations, errors, toast messages

src/lib/i18n.ts Outdated
Comment on lines 9 to 13
session_one: 'Session',
session_other: 'Sessions',
knowledge_one: 'Knowledge',
knowledge_other: 'Knowledge',
settings: 'Settings',
Copy link
Owner

@fmaclen fmaclen Aug 25, 2024

Choose a reason for hiding this comment

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

You don't need sessions and sessionPage, you can just do:

sessions: {
  one: "Session",
  other: "Sessions"
}
settings: {
  one: "Setting",
}

Err, this doesn't work the way I was hoping it would 😞
Leave the naming convention you have already with the section and sectionPage.

Copy link
Owner

Choose a reason for hiding this comment

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

I'll likely end up moving most of the keys as direct children of translation: { }

src/lib/i18n.ts Outdated Show resolved Hide resolved
src/routes/+layout.svelte Outdated Show resolved Hide resolved
@GregoMac1
Copy link
Collaborator Author

@fmaclen I've translated the remaining texts.

And in this example we can keep the <code> tags in the translated strings:

<p>
  {{ If you want to connect to an Ollama server that is not available on <code class="code">localhost</code> or <code class="code">127.0.0.1</code> you can try: }}
  <a>{{ Creating a tunnel }}</a>
  <a>{{ Allowing mixed content }}</a>
</p>

About this quote of this comment, I've tried it in several ways using the escaping options, but I couldn't get to the desired result (image below), so for now I left the unformatted text in the translation.

image

As for the rest of texts with interpolated html tags, please let me know if you'd wish something to be changed.

@GregoMac1 GregoMac1 requested a review from fmaclen August 27, 2024 18:05
@fmaclen
Copy link
Owner

fmaclen commented Aug 27, 2024

I've tried it in several ways using the escaping options, but I couldn't get to the desired result

Try the svelte syntax {@html}, here's an example.

Copy link
Owner

@fmaclen fmaclen left a comment

Choose a reason for hiding this comment

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

I think everything else looks good, once you fix the escaping issue let's get this one merged 👍

@GregoMac1
Copy link
Collaborator Author

Thank you! It worked!

Try the svelte syntax {@html}, here's an example.

About the example, I think you can get rid of the 'hack' by using eslint-disable-next-line instead of eslint-disable-line (check this commit), although I'm not sure if that was exactly the problem.

@GregoMac1 GregoMac1 merged commit eaa1f92 into main Aug 28, 2024
2 checks passed
@GregoMac1 GregoMac1 deleted the localize-components branch August 28, 2024 20:33
@fmaclen
Copy link
Owner

fmaclen commented Aug 30, 2024

🎉 This PR is included in version 0.10.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localize labels in UI components
2 participants