OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

Use i18n Keys

Open Vaishakh-SM opened this issue 1 year ago • 14 comments

End-user friendly description of the problem this fixes or functionality that this introduces

Right now, several instances of plain English language are used in the codebase. Using i18 keys would ensure better inclusivity by enabling users to use different languages.


Link of any specific issues this addresses

Issue#4280

Vaishakh-SM avatar Oct 15 '24 05:10 Vaishakh-SM

Hi @amanape

Does this change seem fine?

If so, I'll make similar changes to the rest of the repo.

Vaishakh-SM avatar Oct 15 '24 05:10 Vaishakh-SM

Yes, that's the correct way to do it. Don't worry about trying to get the other translations in though, you can focus on English only for now.

I recommend converting this PR to a draft if you're thinking of continuing

amanape avatar Oct 15 '24 05:10 amanape

Hi @Vaishakh-SM @amanape , just some questions, to start helping on the process

1. do you think its best to keep all the translations in one file?

As a positive I see everithing is centralized and as a negative the file might grow too large.

2. If so, do you think its worth grouping the elements on the Json structure itself?

I see currently the resources are prefixed, so for example CODE_EDITOR$FILE_SAVED_SUCCESSFULLY, CODE_EDITOR$SAVING_LABEL, CODE_EDITOR$SAVE_LABEL. Another approach used in previews projects I've seen is to group things, for example:

{
    "CODE_EDITOR": {
        "SAVE_LABEL": {
            "en": "Save",
            "zh-CN": "保存",
        }
    }
}

Using the resource would be similar using dot notation t(I18nKey.CODE_EDITOR.SAVE_LABEL)

I understand if probably the better choice is to work as it is, because this change is already big enough and changing the standard would require modify the parts that are already translated; I just wanted to bring it up in case you think something is worth changing before starting.

3. What would be the standard to leave the resources pending to be translated?

If you leave the key missing then the system will take the resource of the default language (English) which I think its a good behavior, but leaving the key missing could also reduce visibility for the pending translations

4. How could I support without generating git conflicts?

This one goes more to you @Vaishakh-SM, Im sorry, I have never co-authored a PR here on github, so, how would be the better way? do I push changes to the same branch? do I create a new branch and PR changes to yours, or how is it better to proceed?. Because we would be modifying the same file I don't want to create git conflicts

Thanks

danicruz0415 avatar Oct 15 '24 20:10 danicruz0415

Hey @danicruz0415, thanks for your input. To answer your questions:

  1. do you think its best to keep all the translations in one file?

No, I do not. The recommended way in the official documentation would be to have a translations file per language. (e.g., en.json contains the English translations, de.json contains the German translations, etc). I definitely think we should look for ways to break down the translation files for the future.

  1. If so, do you think its worth grouping the elements on the Json structure itself?

Yes, that is a good alternative and I can see it improve DX for the time being. As a side note, since the major UI changes, there are actually a lot of translations that are no longer required, but left over. In reality, there are only a few keys currently being used.

  1. What would be the standard to leave the resources pending to be translated?

As is. IF we had individual translation files, it would be easy for other individuals to extend it.

  1. How could I support without generating git conflicts?

I think one strategy would be to agree on certain routes/components to work on, on separate PRs. There will be some conflict since some routes re-use certain components, but it would expected and shouldn't be too difficult to resolve. You can see a basic map of the FE here:

https://github.com/All-Hands-AI/OpenHands/tree/main/frontend#project-structure

Thanks a lot for taking this up! Feel free to open another PR mentioning the routes/components you plan to cover

amanape avatar Oct 16 '24 05:10 amanape

The strategy to avoid conflicts sounds good to me!

@danicruz0415 I'll take up the routes that you aren't once you open up your PR.

Vaishakh-SM avatar Oct 16 '24 05:10 Vaishakh-SM

Hi! I've opened this PR: https://github.com/All-Hands-AI/OpenHands/pull/4464 Sorry for the late response I was a bit bussy; I also had an error running the interface locally.

@Vaishakh-SM if its ok by you I can take the "components/modals" folder @amanape can you take a look at the PR to see if it makes sense? sorry I know its a trivial change but I want to be sure 🙈

I leave the error that I was having, and the solution in case someone stumbles upon it, as googling it did not show many results

'VITE_MOCK_API' is not recognized as an internal or external command

It seems that this syntax where you set up an environment variable before running the command is not recognized by windows PowerShell (see this SO Question). The solution is to remove the env variable (change the "dev" script inside the package.json to "dev": "npm run make-i18n && remix vite:dev" and then use a ".env" file to set the VITE_MOCK_API variable to false

@amanape do you think this is worth mentioning on the troubleshoot section of the readme? It might be that is a trivial error just that I am not too familiar with it, so it took me a little to figure it out jeje

danicruz0415 avatar Oct 18 '24 03:10 danicruz0415

@Vaishakh-SM if its ok by you I can take the "components/modals" folder

Sure! Do you plan on taking up any other folders?

Vaishakh-SM avatar Oct 18 '24 04:10 Vaishakh-SM

Hi @Vaishakh-SM sorry, I wasn't sure which ones needed translation so I mentioned the one I was testing with 🙈 I did a review of the code of all the folders inside the components folder and found the following

🟦 means does not need localization 🟩 means its already localized 🟥 means needs localization

  • 📁 buttons 🟦
  • 📁 chat 🟩
  • 📁 context-menu 🟦
  • 📁 file-explorer 🟩
  • 📁 form 🟥
  • 📁 markdown 🟦
  • 📁 modals 🟥
  • 📁 project-menu 🟥
  • 📁 terminal 🟦
  • ⚛️ account-settings-context-menu.tsx🟥
  • ⚛️ AgentControlBar.tsx🟥
  • ⚛️ AgentStatusBar.tsx 🟩
  • ⚛️ Browser.tsx 🟥
  • ⚛️ container.tsx 🟥
  • ⚛️ controls.tsx 🟦
  • ⚛️ editor-actions.tsx 🟥
  • ⚛️ error-toast.tsx 🟥
  • ⚛️ FileIcons.tsx 🟦
  • ⚛️ FolderIcon.tsx 🟦
  • ⚛️ IconButton.tsx 🟦
  • ⚛️ Jupyter.tsx 🟥
  • ⚛️ suggestion-bubble.tsx 🟦
  • ⚛️ user-avatar.tsx 🟥

If its ok by you I can take the folders ./form ./modals and ./project-menu and you take the files outside the folders. Im not sure if its around the same number of files but I think it should be close >.<

I am not sure if anything outside the components folder needs any localization. As far as I saw it didn't.

danicruz0415 avatar Oct 19 '24 01:10 danicruz0415

If its ok by you I can take the folders ./form ./modals and ./project-menu and you take the files outside the folders. Im not sure if its around the same number of files but I think it should be close >.<

Thanks for the hard work of categorizing the files!

Sorry for the late response. Yes, this seems fine to me. I'll start working on the files outside these folders.

Vaishakh-SM avatar Oct 20 '24 16:10 Vaishakh-SM

Hello @Vaishakh-SM just checking in to see if this is generally on your radar still.

mamoodi avatar Nov 14 '24 16:11 mamoodi

Hello @Vaishakh-SM just checking in to see if this is generally on your radar still.

Hi! Sorry for the delay. Yes this is still under my radar. I've made the changes required on all the other files but wanted to take a second look before raising the PR. I'm hoping to do this sometime in the coming week.

Vaishakh-SM avatar Nov 15 '24 19:11 Vaishakh-SM

@Vaishakh-SM if you need any help or a review let me know! 😄

danicruz0415 avatar Nov 15 '24 19:11 danicruz0415

@Vaishakh-SM if you need any help or a review let me know! 😄

Hey Dani! It seems like some checks are failing because the tests look for alt / text of elements. Did you also face this, if so did you also update the tests?

Vaishakh-SM avatar Nov 25 '24 17:11 Vaishakh-SM

Hey @Vaishakh-SM, there have been a significant amount of app-wide changes and will be a few more by the end of this week. Your branch is out of date and requires these conflicts to be resolved. Unfortunately, since it is a branch on your fork, nobody but you can make write changes to them.

If you're struggling with resolving the conflicts, I would recommend opening a new branch that is synced with the latest changes.

amanape avatar Nov 25 '24 18:11 amanape

Hey @Vaishakh-SM, there have been a significant amount of app-wide changes and will be a few more by the end of this week. Your branch is out of date and requires these conflicts to be resolved. Unfortunately, since it is a branch on your fork, nobody but you can make write changes to them.

If you're struggling with resolving the conflicts, I would recommend opening a new branch that is synced with the latest changes.

Hi @amanape , I've opened PR#5286 which is synced with latest main. Still seems like some checks are failing (if I understand, this is because some tests try to find the element from the alt texts) but I think we can continue the discussion in that PR (If so I'll close this PR)?

Vaishakh-SM avatar Nov 26 '24 17:11 Vaishakh-SM

The tests are failing because the text content has changed. This is expected since you're changing hardcoded values to dynamic ones with the help of the i18n library. It is safe to close this PR

amanape avatar Nov 26 '24 18:11 amanape