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

Use i18n Keys #4397

Closed
wants to merge 10 commits into from
Closed

Conversation

Vaishakh-SM
Copy link
Contributor

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
Copy link
Contributor Author

Hi @amanape

Does this change seem fine?

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

@amanape
Copy link
Member

amanape commented Oct 15, 2024

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

@danicruz0415
Copy link
Contributor

danicruz0415 commented Oct 15, 2024

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

@amanape
Copy link
Member

amanape commented Oct 16, 2024

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

@Vaishakh-SM Vaishakh-SM marked this pull request as draft October 16, 2024 05:29
@Vaishakh-SM
Copy link
Contributor Author

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.

@danicruz0415
Copy link
Contributor

Hi! I've opened this PR: #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

@Vaishakh-SM
Copy link
Contributor Author

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

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

@danicruz0415
Copy link
Contributor

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.

@Vaishakh-SM
Copy link
Contributor Author

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.

This was referenced Nov 3, 2024
@mamoodi
Copy link
Collaborator

mamoodi commented Nov 14, 2024

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

@Vaishakh-SM
Copy link
Contributor Author

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.

@danicruz0415
Copy link
Contributor

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

@Vaishakh-SM
Copy link
Contributor Author

Vaishakh-SM commented Nov 25, 2024

@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?

@amanape
Copy link
Member

amanape commented Nov 25, 2024

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.

@Vaishakh-SM
Copy link
Contributor Author

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)?

@amanape
Copy link
Member

amanape commented Nov 26, 2024

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 amanape closed this Nov 26, 2024
@Vaishakh-SM Vaishakh-SM mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants