Skip to content

Migrate hardcoded strings to i18n vars #523

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

Merged

Conversation

marinaaisa
Copy link
Member

@marinaaisa marinaaisa commented Jan 31, 2023

Bug/issue #102139999, if applicable:

Summary

This PR migrates hardcoded strings into i18n vars, including translation files for Chinese and Japanese.

Dependencies

vue-i18n

Testing

Steps:

  1. Add the following to theme-settings.json file at public folder
{
  "features": {
    "docs": {
      "i18n": {
        "enable": true
      }
    }
  }
}
  1. Run npm run serve
  2. Go to the bottom of the page and change the language in the locale selector
  3. Assert that hardcoded strings are translated in Chinese and Japanese

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

@marinaaisa
Copy link
Member Author

@swift-ci test

@marinaaisa marinaaisa force-pushed the r102139999/hardcoded-strings-i18n branch from bd1c194 to 53c6389 Compare January 31, 2023 19:29
Copy link
Contributor

@dobromir-hristov dobromir-hristov left a comment

Choose a reason for hiding this comment

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

Good job, here are some thoughts:

  1. Stitching sentences in general is a bad idea. Left commends on a few of the places, but I think we should rethink those a bit.
  2. async load translations as future improvement would be nice
  3. use user browser locale as a future improvement as well
  4. How would this tie to the router?

* See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import VueI18n from 'vue-i18n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe move this to the /plugins dir?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I implemented it in the same way as SwiftDocCRenderRouter is. What part of the code would you move to the plugins folder?

@marinaaisa marinaaisa force-pushed the r102139999/hardcoded-strings-i18n branch from 53c6389 to 1e90571 Compare February 1, 2023 13:47
@marinaaisa
Copy link
Member Author

Thanks for review it @dobromir-hristov !

  • Stitching sentences in general is a bad idea. Left commends on a few of the places, but I think we should rethink those a bit.

I agree. I fixed them.

  • async load translations as future improvement would be nice

That would be nice. I haven't implemented it yet because we have very few languages but I'll do it in the future.

  • use user browser locale as a future improvement as well

Yes. We will be working on it in a different PR. It's out of scope for this one.

  • How would this tie to the router?

This is also out of scope for this PR but working on it.

@marinaaisa marinaaisa force-pushed the r102139999/hardcoded-strings-i18n branch from ff0b80f to b0c80c1 Compare February 6, 2023 17:35
@marinaaisa marinaaisa marked this pull request as ready for review February 6, 2023 18:58
@marinaaisa
Copy link
Member Author

@swift-ci test

Copy link
Contributor

@hqhhuang hqhhuang left a comment

Choose a reason for hiding this comment

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

Looking great!! Just left a few minor questions/comments. Do we also need to update the headers to these files? 😅

Copy link
Contributor

@mportiz08 mportiz08 left a comment

Choose a reason for hiding this comment

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

So many changes! 😅 Nice work 🙌

I haven't had time to actually test this very closely yet, but I did a full pass on reviewing all the code (except for the tests).

One general comment I have is that it would be nice if we could come up with a more consistent way of naming the message keys. Maybe we could use the component name as prefixes or something where it's a little more obvious to mentally map the strings to how they're used. You've probably thought a lot about this already and have opinions on the naming scheme I'm guessing.

@marinaaisa
Copy link
Member Author

marinaaisa commented Feb 10, 2023

Thanks for the great feedback, @hqhhuang and @mportiz08 !!

Do we also need to update the headers to these files? 😅

Oh no, I still need to update the headers 🫣
Edit: ✅ done

One general comment I have is that it would be nice if we could come up with a more consistent way of naming the message keys. Maybe we could use the component name as prefixes or something where it's a little more obvious to mentally map the strings to how they're used. You've probably thought a lot about this already and have opinions on the naming scheme I'm guessing.

I'm open to suggestions! Naming is always the most difficult part. Any ideas?

@marinaaisa
Copy link
Member Author

@swift-ci test

@marinaaisa marinaaisa force-pushed the r102139999/hardcoded-strings-i18n branch from 77c4b7f to d444249 Compare March 1, 2023 17:03
@marinaaisa
Copy link
Member Author

@swift-ci test

@marinaaisa marinaaisa force-pushed the r102139999/hardcoded-strings-i18n branch from a653841 to d0f911a Compare March 2, 2023 15:33
@marinaaisa marinaaisa force-pushed the r102139999/hardcoded-strings-i18n branch from d0f911a to 8d88e4c Compare March 2, 2023 15:36
@mportiz08
Copy link
Contributor

@marinaaisa can you sync this with the latest main and resolve the conflicts? Hopefully they're minor and easy to fix. I think we're ready to merge this tomorrow once that's done.

@marinaaisa
Copy link
Member Author

@marinaaisa can you sync this with the latest main and resolve the conflicts? Hopefully they're minor and easy to fix. I think we're ready to merge this tomorrow once that's done.

Done! ✅

@mportiz08
Copy link
Contributor

@swift-ci test

@mportiz08
Copy link
Contributor

@swift-ci test

Copy link
Contributor

@mportiz08 mportiz08 left a comment

Choose a reason for hiding this comment

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

Looks good, but I ran into some integration issues that we'll need to figure out next week before we can merge this. I'll reach out offline.

@mportiz08 mportiz08 merged commit d296a01 into swiftlang:main Mar 6, 2023
@mportiz08 mportiz08 deleted the r102139999/hardcoded-strings-i18n branch March 6, 2023 22:14
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