-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: i18n for react #9
Conversation
@@ -66,7 +66,6 @@ files.forEach((filePath) => { | |||
const dataAsHTMLElement = getElement({ selector: 'div', htmlString: `<div>${rawData}</div>` }) | |||
const { name } = getNameAndSize(filePath) | |||
const title = descriptions[pascalCase(name)] | |||
if (title) dataAsHTMLElement.querySelector('svg').prepend(`<title>${title}</title>`) |
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.
Need to remove it here because we are adding it when building the react. (Will be handled for vue and elements in separate PRs)
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.
With this fix the icons that are built in dist, the optimized ones, will loose title. I've seen that those icons were used in FINN directly. SO those will loose title. Can it be good to at least keep the english one? I know that those should not be used directly but in case they are, maybe we can keep english titles to be on the safe side?
I'll try to find examples of those usages
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.
https://github.schibsted.io/finn/new-construction-frontend/blob/457066bdf756d2aaa189382741661649b8b95a88/packages/ad/src/components/keyInfo/index.jsx this looks like one case of those usages
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.
Hmmm good point. We can wait to merge this until we also handle Vue and Elements... Or wdyt?
Because if I remove this change, then the React icons will have two titles.
"i18n:clear": "node scripts/i18n-helpers/clear-locales.js", | ||
"i18n:get-sorted-locales": "node scripts/i18n-helpers/sort-locales.js", |
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.
When is it time to run those scripts?
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've added some comments in the script files:
/**
* Helper to remove locales folder
* Test that localization files in raw folder are made correctly with the i18n script
*/
This one is nice to keep when we are making changes to i18n, and want to test the the locales folder will be made correctly.
/**
* Helper script to clean up and get a sorted default icon descriptions
*/
This one is useful when we are adding new translations, and want to sort the long file that holds all default English translations. I also used chatGPT to generate this file, and chatgpt (or I 😛) did some mistakes (so there were some duplicates that I wanted to remove.
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.
But, maybe I should remove it from the scripts in package.json. Because they wont be used that much. Wdyt?
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.
Can we sort that long file automatically when we generate it? As I assume we always want to have it sorted
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 long file is not automatically generated 😭 I had to go several rounds with chatGPT to generate it based on the Norwegian titles. So when we add a new icon, we need to add the default English translation to default-icon-description.js
. Then we can run the script to sort the file.
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 approve this one but I'm still not clear on why we do have some scripts and in what cases I could use them. But it will be clear I think as we are starting to use them. SO let's merge and improve if it is needed.
I haven't tested the functionality, only scanned the code changes
# [1.1.0-next.1](v1.0.1-next.1...v1.1.0-next.1) (2023-09-26) ### Features * i18n for react ([#9](#9)) ([10e41bc](10e41bc))
# [1.1.0](v1.0.0...v1.1.0) (2023-10-19) ### Bug Fixes * add 9BA8BA to exchange with currentColor ([#17](#17)) ([bea742a](bea742a)) * add leading slash to dest in crowdin config ([#23](#23)) ([d1170b6](d1170b6)) * add more missing titles ([#35](#35)) ([91f2b78](91f2b78)) * element icons names ([#16](#16)) ([0d550c8](0d550c8)) * Fix bundling ([#20](#20)) ([6ea9874](6ea9874)) * make sure finnish translations aren't gitignored ([#40](#40)) ([5c49b2b](5c49b2b)) * pnpm lockfile ([6529014](6529014)) * set next as default prerelease branch ([#12](#12)) ([bb0be72](bb0be72)) * update crowdin.yaml with leading slash ([bd09d68](bd09d68)) * update Readme text ([156d4ed](156d4ed)) * update script with improved destination in crowdin ([#34](#34)) ([30f4c92](30f4c92)) * use proper language placeholder in translations ([#22](#22)) ([4425b66](4425b66)) ### Features * Generate individual icons ([#39](#39)) ([dd74394](dd74394)) * Handle i18n for elements ([#19](#19)) ([8a27850](8a27850)) * Handle i18n for vue ([#14](#14)) ([2cf4fdb](2cf4fdb)) * i18n for react ([#9](#9)) ([10e41bc](10e41bc)) ### Reverts * Revert "feat:Handle i18n for elements (#15)" (#18) ([6bfee40](6bfee40)), closes [#15](#15) [#18](#18)
This PR adds the localisation files needed to handle i18n and adjusts the react script to add translated titles for the React Icons.
Important files to take a closer look at:
scripts/i18n.js
scripts/output/react.js
The
i18n.js
script generates the following.po
and.mjs
files:However, we are only checking in the English
.po
files, because they are needed to make the Crowdin integration work. (To be fixed in this ticket).The
i18n.js
script also generates thelingui.config
.To make i18n work, we do need the compiled translations (the
.mjs
files) in the React/Vue/Elements icons. Therefore, we need to make sure that we are extracting the messages and compiling the messages before we can run the scripts to generate the icons for the frameworks. That's why I've addedlingui extract && lingui compile
to thebuild:react
,build:vue
andbuild:elements
scripts inpackage.json
. (In theory we only need to do it once).I've modified the
react
script to output files to handle i18n, e.g:The
default-icon-descriptions.js
is based on the Icon-description.yml file. I've used ChatGPT to generate this file, so I've informed Ruth Rostrup that they will need to look over the English translations at some point.Tested that it works in another project: