-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/interface clarifications #513
Conversation
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.
Good work @BeritJanssen. How would you like to proceed? Would you like to figure out the parsing yourself (with help, since you got stuck), or defer that work to someone else?
Some general remarks/questions:
- It looks like you identified all the translatable strings in the templates, but I'm not seeing any similar changes in TypeScript files. Sometimes translatable strings are set in TS and then passed to the template or even injected dynamically. Those need to be passed through a
i18next.t()
function call instead of the#i18n
block helper (by heart, I know you can see an example of this infrontend/src/semantic-search/dropdown-constants.ts
). Did you check for translatable strings in TS modules? - I think the Gulp task that extracts the strings using
i18next-parser
need not be integrated into other tasks. We will need to run it only a few times, manually, because it will generate JSON files that we then commit to source.
Some more specific comments below.
frontend/src/panel-annotation-list/annotation-list-panel-template.hbs
Outdated
Show resolved
Hide resolved
@jgonggrijp I won't be able to work on this this week anymore, so if you have time tomorrow, please do pick it up from here. (And I agree to all your comments.) |
Some parts of some templates seem to have previously escaped attention, especially placeholder, data-tooltip and aria-label attributes. Besides those, there were some changes that were added in parallel on develop, as well as a few pieces where alternative orthography or sentence structure of other languages needed to be taken into account.
Adding these changes here as part of #517 instead of in #513 because the latter is outdated compared to the former with regard to these templates. @JeltevanBoheemen FYI
Adding these changes here as part of #517 instead of in #513 because the latter is outdated compared to the former with regard to these views. This is a follow-up on the previous commit, which regarded the templates. @JeltevanBoheemen FYI
In their infinite wisdom, the i18next-parser team decided to go ESM- only, which obviously doesn't work with a TS gulpfile that is still emulating ESM on top of CommonJS. Sigh.
The intent of the triple braces was to prevent escaping, but handlebars-i18next already disables escaping. After a `gulp clean` I found out that gulp-handlebars was actually tripping over this notation.
4176497
to
34900a9
Compare
9886bf9
to
6e87767
Compare
@BeritJanssen I suspect I'm more or less done with this branch, although I'm a bit fuzzy on what was its original goal. Please help me decide! I have done the following:
I have not done the following (but maybe I should?):
Since translatable strings are everywhere, changes are also everywhere. If you would like to look at the code, the most interesting changes are in the |
// i18next.t('button.sample-sources', 'Sample sources'); | ||
// i18next.t('button.my-items', 'My items'); | ||
// i18next.t('button.sample-items', 'Sample items'); | ||
title: i18next.t(i18nKey, 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.
comments should probably go?
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.
No, they are needed to ensure that i18next-parser doesn't purge these keys on the next scan.
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 you elaborate on this? Why is there a difference in behaviour between commented code and removed 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.
It's not outcommented. It's a "clarification" (for the parser) of what the next line means.
@@ -1,5 +1,5 @@ | |||
<figure class="footer-dh"> | |||
<p>Developed by</p> | |||
<p>{{#i18n 'footer.developed-by'}}Developed by{{/i18n}}</p> | |||
<a href="https://dig.hum.uu.nl" target="_blank"><img class="dhlab" | |||
src="{{static 'image/dighum-logo-blue.svg'}}"></a> |
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.
Should we already proactively change the Footer to only include the UU logo and DHLab name?
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.
If that has been agreed, I suppose we should. I'll defer that to a separate ticket, though.
"begin": "Thank you for signing up. You can now " | ||
} | ||
} | ||
} |
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.
Do we need this 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.
Yes, it prevents the contained strings from becoming empty in the non-old version.
// i18next.t('property.typeHints.base64Binary') | ||
// i18next.t('property.typeHints.gYear') | ||
// i18next.t('property.typeHints.string') | ||
const typesWithHints = ['integer', 'base64Binary', 'gYear', 'string']; |
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.
Stray comments?
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.
Nice reorganization of the labels, and it's a good decision to have the tooltip work on plain text! Cannot click on "approve" since I'm the PR author, but I do approve.
This branch adds tooltips, and loads of
i18n
tags. I also added thei18next-parser
to the package.json, and started writing a gulp script to compile messages. That's where I got stuck. @jgonggrijp : this is the branch I mentioned that might help with requesting translations soon.