-
Notifications
You must be signed in to change notification settings - Fork 232
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
Fixed the issue #1577 i18n Team Loadout UI #2443
Conversation
[frontend] [Sun Sep 15 06:23:46 UTC 2024] - Deployed f8a68fb to https://genshin-optimizer-prs.github.io/pr/2443/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Sep 16 15:21:18 UTC 2024] - Deployed 3ad01e5 to https://genshin-optimizer-prs.github.io/pr/2443/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Sep 17 11:42:08 UTC 2024] - Deployed 4d77dba to https://genshin-optimizer-prs.github.io/pr/2443/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Sep 17 13:39:48 UTC 2024] - Deployed 02e5043 to https://genshin-optimizer-prs.github.io/pr/2443/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Sep 17 23:35:12 UTC 2024] - Deployed bf55576 to https://genshin-optimizer-prs.github.io/pr/2443/frontend (Takes 3-5 minutes after this completes to be available) [Wed Sep 18 00:57:38 UTC 2024] - Deleted deployment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, very consistent tagging for l10n. One minor change for list formatting.
@@ -399,9 +399,17 @@ export default function TabTheorycraft() { | |||
)) | |||
.flatMap((value, index, array) => { | |||
if (index === array.length - 2) | |||
return [value, <span key="and">, and </span>] | |||
return [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be done with more native i18next approach: https://www.i18next.com/translation-function/formatting#list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn’t figure out how to directly pass a JSX element to the t function.😢
It seems that only string array can be passed to the List format. When I passed a JSX element array, it was displayed as [object Object].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, you add a token to the Intl.ListFormat, generate, and then replace the tokens. I think that is a fine solution for now. I am fine with this solution, I'd say you should try to use a less-used unicode character as the token. (maybe the newline character? since this is suppose to be a single line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you mean U+200B or something like that, right?
I tried that, but for some reason I got an 'Uncaught' error and it didn't work.
I'm so sorry, it seems to have been a bug in vscode or something. It worked fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, a very novel solution, nice thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry again and again, I found the cause of the error. I hadn't taken locale differences into account at all.😭
I pushed the wrong fix, I will fix it, and move to the new namespace again tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the PR to a draft for now, you can change it back to ready once you finish cooking
@@ -8,6 +8,45 @@ | |||
"theorycraft": "Theorycraft", | |||
"upopt": "Artifact Upgrader" | |||
}, | |||
"buildCardTip": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really an action to change, just more of an observation:
originally the bulk of character related content was on the character page, hence page_character
, now, most of that is happening in the team page, hence page_team
. However, things like builds and loadouts are actually shared between the two pages, since the character page has become a loadout/team manager. I am inclined to keep associated l10n in the same json, rather than splitting them by page, so I wonder if it makes sense to create separate l10n namespaces, such as build.json
and loadout.json
for l10ns specific to those concepts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macerator-yaro I discussed with Tech Lead, I think it would make sense to divide up the l10n into separate files. Can you move the build related terms to build.json
, and loadout related terms to loadout.json
?
Terms have been moved to new namespaces. I'm not sure if this is the right way to divide it, but I gave it a try anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last bit of namespacing change, I appreciate all the work so far.
@@ -121,5 +121,11 @@ | |||
"rvSliderBtn": { | |||
"maximum": "MRV", | |||
"current": "RV" | |||
}, | |||
"optExcludeModal": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a optimizer config, should go to page_character_optimizer.json
"generatedOn": "Build generated on: ", | ||
"selectChar": "Select a character to generate builds.", | ||
"clearBuildsBtn": "Clear Builds", | ||
"createBuildTc": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"createBuildTc", "createBuildReal" and "buildDisplay" should go to build.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for all your work!
Describe your changes
Replaced the raw-string sections with
t
function orTrans
component, and wrote them into the corresponding files inlibs/gi/localization/assets/locales/en
.Below are the changes:
marked with
TODO: Translation
and raw-string sections on the Teams, Weapons, and Characters pagesIf this PR is successfully merged, please upload the translations into POEditor.
Issue or discord link
fixed #1577
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-ci
locally to validate format and lint.