-
Notifications
You must be signed in to change notification settings - Fork 78
chore(web-forms#825): allow for forms app translation in transifex #1692
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?
Changes from all commits
8404374
1b3eb5c
7e09330
99c3d3a
ec9d80d
44ac94f
11443bf
dd27358
a551eed
18dd6ff
7fd1124
b81307b
ff8f41c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@getodk/forms": patch | ||
| --- | ||
|
|
||
| Added hooks to import and export translations with transifex |
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided not to move this file, but welcome to review feedback. It should probably go in the root/bin but because the web-forms package has a completely different approach it might be confusing there too.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The root bin/ makes sense to me too. Maybe we could add code comments to try to reduce the confusion. Or what if we added a new bin/ directory to apps/? I'm also happy to leave it in apps/central/ and see how that feels over time. We can always change it later.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is my preferred approach. I'm hoping the two solutions standardise over time at which point the winning solution can end up at the root. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1087,10 +1087,10 @@ const writeTranslations = ( | |
| } | ||
|
|
||
| translations.delete('component'); | ||
| fs.writeFileSync( | ||
| `${localesDir}/${locale}.json`, | ||
| JSON.stringify(translations, null, 2) | ||
| ); | ||
| const content = JSON.stringify(translations, null, 2); | ||
| if (content != null && content !== '{}') { | ||
| fs.writeFileSync(`${localesDir}/${locale}.json`, content); | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this because forms doesn't support all the languages that central does. Without this the script fails trying to write
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about a case where a language used to have translations, but then the translations became outdated because the source messages changed, such that the language is no longer translated? In such a case, I think we would still want to write I don't really understand how this problem comes up given that bin/transifex/destructure.js is only iterating over locales with a corresponding transifex_*.json file. I think I understand that we need to handle the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just to add a little more here, I'm wondering:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also happy to continue discussing this in a follow-up issue.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Circling back to this, I wonder if this has less to do with mismatching sets of supported locales and more to do with the absence of messages outside components. That absence isn't something that happens in Central, so the script might not handle that case well.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's exactly the problem. Because web-forms doesn't have any it doesn't need files in the src/locales dir and it feels silly to add a blank one per locale. This will probably change in future but this feels like a sensible guard regardless. |
||
| }; | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,9 +4,16 @@ | |||||||||||
| "private": true, | ||||||||||||
| "type": "module", | ||||||||||||
| "scripts": { | ||||||||||||
| "lint": "eslint . --report-unused-disable-directives --cache --max-warnings 0", | ||||||||||||
| "lint:fix": "eslint . --fix --report-unused-disable-directives --cache --max-warnings 0", | ||||||||||||
| "test": "vitest run" | ||||||||||||
| "eslint": "eslint . --report-unused-disable-directives --cache --max-warnings 0", | ||||||||||||
| "eslint:fix": "eslint . --fix --report-unused-disable-directives --cache --max-warnings 0", | ||||||||||||
| "lint": "npm-run-all eslint transifex:lint", | ||||||||||||
| "lint:fix": "npm-run-all eslint:fix transifex:fix", | ||||||||||||
| "test": "vitest run", | ||||||||||||
| "transifex:destructure": "node ../central/bin/transifex/destructure.js", | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm interested in understanding this case better and experimenting. Could you please point me to the code in the component?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good example: central-frontend/apps/forms/src/components/web-form-renderer.vue Lines 326 to 330 in 78b49bc
Here you have a translation string, with some HTML inside it, and another translated string inside that.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! I'm going to look at it in more detail
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking I should wait for @latin-panda to take a look at this important use case and to consider the overall approach. Does that sound right? Or should I proceed with review with an eye toward trying to merge soon?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay! I should be able to take a closer look tomorrow. I plan to explore how we can support this using the translation library used by web forms.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I confirm that this ⬆️ addresses all translation cases needed for
We just need to host the library in a common location accessible to both apps/forms and packages/web-forms. Overall, I recommend this approach; it is straightforward, no big effort, and keeps the translation solution lightweight and up to date with the latest Message Format, without complex parsing or wrappers @garethbowen, can we reframe this issue getodk/web-forms#855 to focus on using this approach combined with the WF method for determining the language, which is already part of the library?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the refactor POC and that issue are better kept separate, but it depends how everything comes together. For example, if the user selects a language then you don't need an event if and only if the bundling is done in such a way that both the app and component shared the same instance of the translation library. Bundling is very fragile and hard to spot when it breaks so it may need an e2e regression test to ensure the app and the component languages stay in sync. Even if that is true for our use case, for 3rd party users of the component they may have a completely separate translation library for their app, so they will still benefit from the event. In that case I would keep 855 around as a low priority, and have a new issue for the refactor that can be solved now. Either way before the regression work is done I think it would be good to have a discussion with someone from Central, perhaps you me and Matt, because ultimately we should be aiming for all of central-frontend to standardise on one approach or the other. There's no point switching the forms app as in your POC, and then turning around to switching back to vue-intl if we decide that's the right approach later, right?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to have that conversation with you and Matt. I agree we shouldn't rework it twice. However, apps/forms is still small, and a good opportunity to test the translation library :) I'd frame the question as "does web-forms' approach hold up as the standard" rather than "should we align on vue-i18n". Looking at what each stack costs today:
That's a meaningful reduction in custom infrastructure, and it's aligned with the ICU/CLDR standard rather than a library-specific model. So my honest view is that going the other direction would be trading a lighter, standards-based implementation for a heavier custom one. I'd want to see a specific case where vue-i18n comes out ahead before treating it as the default. The migration cost of apps/central is real and worth sizing, but there are strategies we can consider to keep it manageable (good things to dig into in our conversation). This project isn't in maintenance mode; it's still growing in features, so it's nice for the tech stack to move forward too. Let me know when works!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Conversation continued on Slack...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just adding some thoughts here!
Looking forward to discussing this with @latin-panda on a call soon. 👍
I feel like there's probably more than one question or one frame to consider here. To me, it's a complex set of tradeoffs for us to discuss.
Writing the tooling was a cost for sure, but we bore most of that cost in 2020. There would also be a cost to switching. To me, sizing that cost is an important question to discuss.
sentence-separator.vue isn't really related to component interpolation. It's more about combining two separate messages, where one message is conditional. Here's an example of where sentence-separator.vue is helpful, not involving component interpolation: {{ $t('sentence1') }}
<template v-if="someCondition"><sentence-separator/>{{ $t('sentence2') }}</template>I'd be curious about what a solution to this case would look like in Web Forms.
I'm sure there are some aspects of the migration that would be quite doable. 👍 One specific question I have is how we would approach existing uses of component interpolation. Would we try to merge existing Central messages, which are currently split up for component interpolation? That would discard existing translations unless we also automated the merge of translations within Transifex. Alternatively, would we try to preserve the current split messages by writing our own version of Vue I18n's Another thing on my mind is the timing in which translations are loaded. Central has many async components, and it's important that not all translations are loaded at once. Vue I18n gives us that functionality out of the box. |
||||||||||||
| "transifex:fix": "node ../central/bin/transifex/restructure.js > transifex/strings_en.json", | ||||||||||||
| "transifex:lint": "bash -c 'diff transifex/strings_en.json <(node ../central/bin/transifex/restructure.js)'", | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alex just changed this in #1650.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I'll rebase.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately his changes are relative so don't work if ported here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah interesting. Maybe worth filing an issue about to come back to later? Let me also CC @alxndrsn
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't understand 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I copy your changes in the wf apps dir as is then it lints central translations against web-forms translations which fails. I'll have a look at modifying the script sometime...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we talking about bc40617? |
||||||||||||
| "translations:pull": "cd ../.. && tx pull -r web_forms.app -a -f --mode translator", | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would love to get
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was going for consistency with web-forms package. @latin-panda What do you think about the rename for both wf package and app?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's okay to align the command names. Here, I was thinking of making it about the feature name rather than the tool name. |
||||||||||||
| "translations:push": "cd ../.. && tx push -r web_forms.app -s" | ||||||||||||
| }, | ||||||||||||
| "dependencies": { | ||||||||||||
| "primevue": "4.3.3", | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,15 +234,15 @@ load(); | |
| <template v-if="!loadingState"> | ||
| <Dialog modal v-if="errorCode === 404" :draggable="false" :closable="false" :visible="true"> | ||
| <template #header> | ||
| {{ $t('formNotFound') }} | ||
| {{ $t('formNotFound.title') }} | ||
| </template> | ||
| <template #default> | ||
| {{ $t('formNotFound.body') }} | ||
| </template> | ||
| </Dialog> | ||
| <Dialog modal v-else-if="errorCode" :draggable="false" :closable="false" :visible="true"> | ||
| <template #header> | ||
| {{ $t('errorNotProblem') }} | ||
| {{ $t('errorNotProblem.title') }} | ||
| </template> | ||
| <template #default> | ||
| <p>{{ $t('errorNotProblem.body') }}</p> | ||
|
|
@@ -273,11 +273,15 @@ load(); | |
| <i18n lang="json5"> | ||
| { | ||
| "en": { | ||
| "formNotFound": "Unable to open form", | ||
| "formNotFound.body": "Please check that the link is correct. The form may no longer be available, or your access may have expired. If the problem continues, contact the person who sent you the form link.", | ||
| "errorNotProblem": "Something went wrong", | ||
| "errorNotProblem.body": "Please try again later. If the problem continues, contact the person who sent you the form link.", | ||
| "errorNotProblem.status": "Error code: {status}", | ||
| "formNotFound": { | ||
| "title": "Unable to open form", | ||
| "body": "Please check that the link is correct. The form may no longer be available, or your access may have expired. If the problem continues, contact the person who sent you the form link." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Something went wrong", | ||
| "body": "Please try again later. If the problem continues, contact the person who sent you the form link.", | ||
| "status": "Error code: {status}" | ||
| } | ||
|
Comment on lines
-276
to
+284
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't review this sort of change closely, but I do support the removal of keys containing |
||
| } | ||
| } | ||
| </i18n> | ||
|
|
@@ -286,50 +290,82 @@ load(); | |
| <i18n> | ||
| { | ||
| "de": { | ||
| "formNotFound": "Für diese URL wurde kein Formular gefunden, bitte überprüfen Sie dies noch einmal.", | ||
| "errorNotProblem": "Etwas ging schief", | ||
| "errorNotProblem.status": "Fehlercode {status}", | ||
| "formNotFound": { | ||
| "title": "Für diese URL wurde kein Formular gefunden, bitte überprüfen Sie dies noch einmal." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Etwas ging schief", | ||
| "status": "Fehlercode {status}" | ||
| } | ||
| }, | ||
| "es": { | ||
| "formNotFound": "No se puede abrir el formulario", | ||
| "formNotFound.body": "Por favor, verifique que el enlace es correcto. Es posible que el formulario ya no esté disponible o que su acceso haya expirado. Si el problema persiste, comuníquese con la persona que le envió el enlace del formulario.", | ||
| "errorNotProblem": "Algo salió mal", | ||
| "errorNotProblem.body": "Inténtelo de nuevo más tarde. Si el problema persiste, comuníquese con la persona que le envió el enlace del formulario.", | ||
| "errorNotProblem.status": "Código de error: {status}", | ||
| "formNotFound": { | ||
| "title": "No se puede abrir el formulario", | ||
| "body": "Por favor, verifique que el enlace es correcto. Es posible que el formulario ya no esté disponible o que su acceso haya expirado. Si el problema persiste, comuníquese con la persona que le envió el enlace del formulario." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Algo salió mal", | ||
| "body": "Inténtelo de nuevo más tarde. Si el problema persiste, comuníquese con la persona que le envió el enlace del formulario.", | ||
| "status": "Código de error: {status}" | ||
| } | ||
| }, | ||
| "fr": { | ||
| "formNotFound": "Impossible d'ouvrir le formulaire", | ||
| "formNotFound.body": "Veuillez vérifier que le lien est correct. Il est possible que le formulaire ne soit plus disponible ou que votre accès ait expiré. Si le problème persiste, veuillez contacter la personne qui vous a envoyé le lien vers le formulaire.", | ||
| "errorNotProblem": "Quelque-chose s'est mal passé", | ||
| "errorNotProblem.body": "Veuillez réessayer plus tard. Si le problème persiste, veuillez contacter la personne qui vous a envoyé le lien vers le formulaire.", | ||
| "errorNotProblem.status": "Code d'erreur: {status}", | ||
| "formNotFound": { | ||
| "title": "Impossible d'ouvrir le formulaire", | ||
| "body": "Veuillez vérifier que le lien est correct. Il est possible que le formulaire ne soit plus disponible ou que votre accès ait expiré. Si le problème persiste, veuillez contacter la personne qui vous a envoyé le lien vers le formulaire." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Quelque-chose s'est mal passé", | ||
| "body": "Veuillez réessayer plus tard. Si le problème persiste, veuillez contacter la personne qui vous a envoyé le lien vers le formulaire.", | ||
| "status": "Code d'erreur: {status}" | ||
| } | ||
| }, | ||
| "id": { | ||
| "formNotFound": "Tidak dapat membuka formulir", | ||
| "formNotFound.body": "Pastikan tautan sudah benar. Formulir mungkin sudah tidak tersedia, atau akses Anda mungkin telah kedaluwarsa. Jika masalah berlanjut, hubungi orang yang mengirimkan tautan formulir tersebut kepada Anda.", | ||
| "errorNotProblem": "Terjadi kesalahan", | ||
| "errorNotProblem.body": "Coba lagi nanti. Jika masalah berlanjut, hubungi orang yang mengirimkan tautan formulir tersebut kepada Anda.", | ||
| "errorNotProblem.status": "Kode error: {status}", | ||
| "formNotFound": { | ||
| "title": "Tidak dapat membuka formulir", | ||
| "body": "Pastikan tautan sudah benar. Formulir mungkin sudah tidak tersedia, atau akses Anda mungkin telah kedaluwarsa. Jika masalah berlanjut, hubungi orang yang mengirimkan tautan formulir tersebut kepada Anda." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Terjadi kesalahan", | ||
| "body": "Coba lagi nanti. Jika masalah berlanjut, hubungi orang yang mengirimkan tautan formulir tersebut kepada Anda.", | ||
| "status": "Kode error: {status}" | ||
| } | ||
| }, | ||
| "it": { | ||
| "formNotFound": "Non è stato trovato alcun modulo con questo URL, si prega di ricontrollare.", | ||
| "errorNotProblem": "Qualcosa è andato storto", | ||
| "errorNotProblem.status": "Codice error {status}", | ||
| "formNotFound": { | ||
| "title": "Non è stato trovato alcun modulo con questo URL, si prega di ricontrollare." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Qualcosa è andato storto", | ||
| "status": "Codice error {status}" | ||
| } | ||
| }, | ||
| "pt": { | ||
| "formNotFound": "Nenhum Formulário encontrado com esse endereço, por favor verifique.", | ||
| "errorNotProblem": "Algo deu errado", | ||
| "errorNotProblem.status": "Código de erro {status}", | ||
| "formNotFound": { | ||
| "title": "Nenhum Formulário encontrado com esse endereço, por favor verifique." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Algo deu errado", | ||
| "status": "Código de erro {status}" | ||
| } | ||
| }, | ||
| "zh": { | ||
| "formNotFound": "未找到与此URL对应的表单,请仔细核对。", | ||
| "errorNotProblem": "出现错误:错误代码", | ||
| "errorNotProblem.status": "{status}", | ||
| "formNotFound": { | ||
| "title": "未找到与此URL对应的表单,请仔细核对。" | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "出现错误:错误代码", | ||
| "status": "{status}" | ||
| } | ||
| }, | ||
| "zh-Hant": { | ||
| "formNotFound": "此 URL 未找到表單,請仔細檢查。", | ||
| "errorNotProblem": "出了點問題:錯誤代碼", | ||
| "errorNotProblem.status": "{status}", | ||
| "formNotFound": { | ||
| "title": "此 URL 未找到表單,請仔細檢查。" | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "出了點問題:錯誤代碼", | ||
| "status": "{status}" | ||
| } | ||
| } | ||
| } | ||
| </i18n> | ||

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 I was last updating Central's translations, I got a permissions error when running
tx pullbecause I don't have access to the Web Forms project in Transifex. I think that's fine, I don't feel like I need that access, but it made me wonder how best to approach that step. To make progress, I temporarily removed Web Forms from .tx/config.I like what you're doing below with
tx pull -r web_forms.app. Maybe we should do something similar for Central, adding a command to Central's package.json.Another idea is that we could have multiple .tx/config files in different directories.
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.
Because we release together now I think it'd make sense for all the translations to be able to be updated together which means whoever's doing the release needs permissions on all projects. I'll follow up with Hélène.
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 was wondering about that now that we have a shared timeline/release. I could see it being useful to update all the translations together.