-
Notifications
You must be signed in to change notification settings - Fork 168
Sync translations #1987
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
base: master
Are you sure you want to change the base?
Sync translations #1987
Conversation
scripts/diff-lang.js will check translation coverage of languages against src/locales/en, however src/locales/en has fallen out of sync with the source code. This new test aims to restore the sync. There are 52 errors identified by the test. They will be addressed in subsequent commits.
This text was removed from source in "Remove the search-strains UI" (ece581d) with no replacement.
This allows detection through the new translations.test.js.
The new casing is actually used in the code.
jameshadfield
left a comment
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.
Briefly read through things as it popped up on my notifications. This is a good direction to head in tho!
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 haven't worked my way through translations.test.js yet, but what is it about the previous code in this commit that wasn't able to be picked up by traversing the AST?
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.
The translation test file traverses the AST to find instances of t() and the first argument passed to it. If the argument is a literal string, we can get the value simply through traversal. If the argument is a variable, traversal is insufficient to determine the value and some form of evaluation would be necessary. It may be possible, but seems like a fundamentally different approach so I haven't tried it.
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.
Makes sense - thanks!
| "Collection date": "Collection date", | ||
| "Inferred collection date": "Inferred collection date", | ||
| "Inferred Date": "Inferred Date", | ||
| "Inferred date": "Inferred date", |
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.
We use both...
auspice/src/components/tree/infoPanels/hover.js
Lines 53 to 54 in c4768dd
| const dateDescription = inferred ? 'Inferred Date' : 'Date'; | |
| elements.push(<InfoLine name={t(dateDescription)+":"} value={date} key="date"/>); |
auspice/src/components/tree/infoPanels/click.js
Lines 180 to 186 in c4768dd
| const dateDescription = isTerminal ? | |
| (inferred ? "Inferred collection date" : "Collection date") : | |
| "Inferred date"; // hardcoded assumption that internal nodes are inferred | |
| return ( | |
| <> | |
| {item(t(dateDescription), date)} |
I prefer the capitalised form (i.e. prefer the style of the hover code over the click code).
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.
Oh thanks, not sure how I missed that yesterday. I'll replace e69bec2 with a commit that swaps Inferred date to Inferred Date.
Description of proposed changes
scripts/diff-lang.jswill check translation coverage of languages againstsrc/locales/en, howeversrc/locales/enhas fallen out of sync with the source code. I've added a new test to uncover what is out of sync (52 errors) and will address them in this PR since they are now exposed as failing tests in CI.Related issue(s)
Closes #1960
Checklist