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

Implement Translations #252

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Implement Translations #252

wants to merge 8 commits into from

Conversation

optim77
Copy link

@optim77 optim77 commented Jan 23, 2025

  • Added lingui to the project
  • implemented the translation of the entire application
  • Translated it into Polish
  • Added a button to select language to options (select is written manually in the future you we generate it)

One walkaround:
When switching languages, the content of the options is not translated because the items are not updated, so when switching languages I force a reload of the application (this can be fixed in the future)

I didn't generate production files with translations.

Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for ytify ready!

Name Link
🔨 Latest commit 50d3a91
🔍 Latest deploy log https://app.netlify.com/sites/ytify/deploys/67935e9dedc3310008936ca2
😎 Deploy Preview https://deploy-preview-252--ytify.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 89
Accessibility: 97
Best Practices: 92
SEO: 83
PWA: 100
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@n-ce n-ce self-requested a review January 23, 2025 16:02
@n-ce n-ce marked this pull request as ready for review January 23, 2025 16:02
Copy link
Owner

@n-ce n-ce 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 insane! I didn't expect to have such wide code coverage! Amazing and efficient work. 🤩

Initial over the top suggestions :

  • The Language Changer should be in top of settings preferably below or above Use Custom Instance as a <Selector>
  • Translation for Settings headings
  • Missing 'Now Playing' and 'Channel' text from empty Player
  • It seems to be based on 7.6.6, so we have to update to 7.6.7 Sync with 7.6.7 optim77/ytify#1
  • Add .idea folder to .gitignore to avoid adding to repo 8b02f81

@n-ce
Copy link
Owner

n-ce commented Jan 23, 2025

Secondary Notes

  • Can we implement the lingui config file inside the vite config? There does exist a vite plugin.
  • It seems we are not making use of some lingui modules, can we remove them?
  • Can we use json for translation files instead of ts ?
  • What are the po files?
  • The full page reload is actually okay, some of the settings also require a reload. It keeps the code small and avoids unnecessary application fatigue.

@optim77
Copy link
Author

optim77 commented Jan 23, 2025

I have implemented your suggestions

As for the questions

  • lingui must have its own separate config
  • I'll remove the unnecessary modules I've uploaded, but there are still others that depcheck detects:
Unused dependencies
* sortablejs
Unused devDependencies
* @types/sortablejs
* autoprefixer
* eruda
* vite-plugin-pwa
* vite-plugin-solid
  • we can store translations in json files, but since we are using type, .ts files must exist anyway so it is better to limit the files to .ts
  • .po files are the standard files for translations, lingui likes them the most, but as with json, .po files are also an extra file before .ts so I deleted them too

in addition, the current font does not support diacritics, so they look bad

@n-ce
Copy link
Owner

n-ce commented Jan 23, 2025

there are still others that depcheck detects:

Unused dependencies
* sortablejs
Unused devDependencies
* @types/sortablejs
* autoprefixer
* eruda
* vite-plugin-pwa
* vite-plugin-solid

These are actually used, so it's fine.

in addition, the current font does not support diacritics, so they look bad

Is there any way to deal with this? Noto Sans is a popular font so I think it supports all kinds of facilities, the fault might be in my current implementation. https://fonts.google.com/noto/specimen/Noto+Sans

Additionally,

  • can you merge the pending pull request ?
  • I think we can merge the i18.ts and translateHTML.ts into a single i18.ts file
  • do remove the .idea folder from git via git rm -r .idea
  • ununsed callChangeLanguage function at the bottom in Settings.tsx

@optim77
Copy link
Author

optim77 commented Jan 23, 2025

  • moved code from translateHTML.ts to i18.ts
  • deleted .idea
  • deleted callChangeLanguage
  • for the font, it can be changed to Noto Sans Extended which looks a little different but I would leave it until this is resolved Improve Styling #249
  • I don't think I have merge permissions because I don't see the button anywhere (I've never done this before)

@n-ce
Copy link
Owner

n-ce commented Jan 24, 2025

(I've never done this before)

🤯 what that is outstanding, it's here btw : optim77#1

@n-ce n-ce changed the base branch from main to dev January 24, 2025 09:41
@n-ce n-ce added enhancement New feature or request Javascript labels Jan 24, 2025
@n-ce n-ce linked an issue Jan 24, 2025 that may be closed by this pull request
Copy link
Owner

@n-ce n-ce left a comment

Choose a reason for hiding this comment

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

  • in lingui.config.ts , the format mentioned is po, but we are not using the po files anymore so is it necessary?
  • in src/locales I think we can remove folders and use files directly as en.ts, pl.ts, inside lingui.config.ts changing path to src/locales/{locale}
  • in ActionsMenu.tsx ,
    • titles are now using <span> element, i think they can be plain jsx expressions by using the { } syntax
    • View Lyrics isn't translated.
  • in Settings.tsx, from the new update Prefer Stable Volume has not been translated.

Apart from this we are good enough. We've managed to do this in just 350 lines of code including the english and poland translation files.
Also do you wish to maintain the polish translations after this PR has been merged?

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Translations
2 participants