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: translate UI to Spanish #179

Merged
merged 22 commits into from
Sep 12, 2024
Merged

feat: translate UI to Spanish #179

merged 22 commits into from
Sep 12, 2024

Conversation

GregoMac1
Copy link
Collaborator

No description provided.

@GregoMac1 GregoMac1 self-assigned this Sep 6, 2024
@GregoMac1 GregoMac1 linked an issue Sep 6, 2024 that may be closed by this pull request
5 tasks
@GregoMac1
Copy link
Collaborator Author

@fmaclen for now I've added the translations to spanish. I still have to implement the switch between languages.

I've used Claude to give me advice on some of them. I still have doubts on a few ones, for example 'Prompt'. What do you think?

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.

@GregoMac1 this translations look good, the one for prompt is kinda weird but it's fine for now.

I edited the original issue and added a short list of tasks we should include in this PR:
#151

src/i18n/es/index.ts Outdated Show resolved Hide resolved
@GregoMac1 GregoMac1 marked this pull request as ready for review September 9, 2024 22:17
@GregoMac1
Copy link
Collaborator Author

@fmaclen as the settings page is divided into categories, I added a 'Preferences' category to contain the language switch input, but feel free to change/remove it.

@GregoMac1 GregoMac1 requested a review from fmaclen September 9, 2024 22:18
Comment on lines 31 to 32
if (!$settingsStore.userLanguage)
$settingsStore.userLanguage = detectLocale('en', ['en', 'es']) as Locales;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

detectLocale accepts a fallback language as first parameter, and a list of available locales as second parameter. Ref

Copy link

cloudflare-workers-and-pages bot commented Sep 10, 2024

Deploying hollama with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3ad39a9
Status: ✅  Deploy successful!
Preview URL: https://985c2a26.hollama.pages.dev
Branch Preview URL: https://151-translate-to-spanish.hollama.pages.dev

View logs

@fmaclen fmaclen changed the title chore: translate localized components to spanish feat: translate UI to Spanish Sep 10, 2024
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.

This is pretty good so far 👍
Left you a couple of minor changes.

Also pushed a few styling fixes because in Spanish strings are often much longer and the UI was wrapping weirdly.

Last but not least, we needs some tests:

  • User can set the language preference to es, and back to en.
  • If the user's browser is in es make sure that's loaded by default, I think you can achieve this by emulating the test options, thought instead of defining a config we should test.use()
  • Update the snapshots npx playwright test --update-snapshots

src/routes/settings/Language.svelte Outdated Show resolved Hide resolved
Comment on lines 24 to 33
<FieldSelect
name="language"
label={$LL.language()}
bind:value
options={[
{ value: 'en', option: 'English' },
{ value: 'es', option: 'Español' }
]}
/>
</Fieldset>
Copy link
Owner

@fmaclen fmaclen Sep 10, 2024

Choose a reason for hiding this comment

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

This looks good, the only thing we are missing here is that we should set the default state for value, right now the field is empty when the page loads.

<FieldSelect> has this issue where we are hard-coding an empty <option></option> which doesn't make sense for this field, but I'm currently doing a big refactor of <FieldSelect> in #181 so don't worry about fixing that. I'll fill you in on the thought process behind that next time we talk.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awesome!

Copy link
Owner

Choose a reason for hiding this comment

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

@GregoMac1 we still need to do the first part of this comment which is setting the default value.

And it'd probably be good to add assertions for the default values in both of the tests.

Copy link
Owner

Choose a reason for hiding this comment

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

I now realize that my comment was a bit ambiguous, I meant not to worry about fixing the hardcoded empty value, but we still need a default value 😅

src/routes/settings/+page.svelte Outdated Show resolved Hide resolved
src/i18n/es/index.ts Outdated Show resolved Hide resolved
src/i18n/es/index.ts Outdated Show resolved Hide resolved
@GregoMac1
Copy link
Collaborator Author

@fmaclen I've added the tests you mentioned.

Could you please try updating the snapshots? I've already done so but the test is still failing. Thanks!

@GregoMac1 GregoMac1 requested a review from fmaclen September 10, 2024 19:25
@fmaclen
Copy link
Owner

fmaclen commented Sep 10, 2024

Could you please try updating the snapshots?

Done!

@GregoMac1
Copy link
Collaborator Author

@fmaclen I've updated the test, so I think this would be ready to be merged.

However, the snapshots test is still failing. I updated the snapshots and the test passes on my computer, but the GH Action fails. I don't really know how to debug this further.

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.

LGTM 🚀

@fmaclen
Copy link
Owner

fmaclen commented Sep 12, 2024

the snapshots test is still failing

@GregoMac1 I think the issue with the snapshots is still indeed the antialias and how sensitive the config is:

docs.test.ts:15:1 › seed data and take screenshots for README.md 
Error: Screenshot comparison failed:
863 pixels (ratio 0.01 of all image pixels) are different.

I increased it to 900 pixels, hopefully this is strikes a good balance for catching actual UI changes and not false positive.

@GregoMac1
Copy link
Collaborator Author

Awesome! Thanks!

@GregoMac1 GregoMac1 merged commit 39268e5 into main Sep 12, 2024
2 checks passed
@GregoMac1 GregoMac1 deleted the 151-translate-to-spanish branch September 12, 2024 01:54
@fmaclen
Copy link
Owner

fmaclen commented Sep 12, 2024

🎉 This PR is included in version 0.13.0 🎉

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.

Translate localized component labels to Spanish
2 participants