UI for form languages and translations #1913
Conversation
There was a problem hiding this comment.
Some thoughts:
- it would be nice when adding a language to have ENTER key work
-
don't we want to suggest users some real world language-code pairs?No - when opening "Translations table" in modal, it would be nice to have "BACK" button
- page input in "Translations table" changes height when empty
- error when trying to delete language:
translationSettings.es6:343 Uncaught TypeError: Cannot read property 'length' of undefined at TranslationSettings.deleteTranslations (translationSettings.es6:343) at TranslationSettings.deleteTranslations (createPrototypeProxy.js:44) at TranslationSettings.deleteLanguage (translationSettings.es6:270) at TranslationSettings.deleteLanguage (createPrototypeProxy.js:44)- language is still there, but translations table is empty - I had form with 1 question, I set default language to "English (en)" and added "Polski (pl)", then translated one string from the "Translations table". Then in form editor I added second question and saved. Now default language is "Unnamed language"
- When I delete the only question language disappears
- "SAVE CHANGES" button from "Translations table" could have pending state and be disabled for a while after clicking
- there is no message in "Manage languages" if form has 0 questions
- I have one non-default language and one translated question. I go to form editor and change the name of the question. Now the non-default language is completely removed.
- Is it ok to allow adding multiple languages with the same code? You can even define same language multiple times. I believe it breaks form in Enketo
- Templates should have translations!
- I don't see language selector in Enketo for "Online-Offline (multiple submission)"
-
Should we have ability to edit translations from Form Editor?Better not yetturns out you can't manage languages, but you can manage translated labels already
jsapp/js/components/formLanding.es6
Outdated
| return false; | ||
|
|
||
| return ( | ||
| <bem.FormView__cell m={['columns', 'padding', 'bordertop', 'languages']}> |
There was a problem hiding this comment.
I know this was already there, but modifier named padding or bordertop seems like a Bad Code™
jsapp/js/components/formLanding.es6
Outdated
| })} | ||
| </bem.FormView__cell> | ||
| {canEdit && | ||
| <bem.FormView__cell m='languages-col2'> |
There was a problem hiding this comment.
maybe just languages-col and :nth-of-type() in css would be more maintainable?
jsapp/js/components/formLanding.es6
Outdated
| languagesModal (evt) { | ||
| evt.preventDefault(); | ||
| stores.pageState.showModal({ | ||
| type: 'form-languages', |
There was a problem hiding this comment.
This should be from MODAL_TYPES from constants.es6
jsapp/js/components/formLanding.es6
Outdated
| @@ -143,6 +109,13 @@ export class FormLanding extends React.Component { | |||
| asset: this.state | |||
| }); | |||
| } | |||
| languagesModal (evt) { | |||
There was a problem hiding this comment.
showLanguagesModal sounds more like action
| } | ||
| } | ||
| launchTranslationTableModal(e) { | ||
| let index = parseInt($(e.target).closest('[data-index]').get(0).getAttribute('data-index')), |
There was a problem hiding this comment.
wouldn't it be shorter and simpler if you would use evt.currentTarget?
| asset: asset, | ||
| langIndex: index | ||
| }); | ||
| }, 300); |
There was a problem hiding this comment.
Is it possible to wait for some event/action rather than random ms value? I'm ok with that if too hard to implement
There was a problem hiding this comment.
I made this into stores.pageState.switchModal method with timeout 0 - hacky but couldn't find any better and quick solution
| this.updateAsset(content); | ||
| } | ||
| deleteLanguage(e) { | ||
| let index = parseInt($(e.target).closest('[data-index]').get(0).getAttribute('data-index')); |
There was a problem hiding this comment.
same as above: currentTarget could be better here
| deleteLanguage(e) { | ||
| let index = parseInt($(e.target).closest('[data-index]').get(0).getAttribute('data-index')); | ||
| let content = this.props.asset.content; | ||
| content = this.deleteTranslations(content, index); |
There was a problem hiding this comment.
const content = this.deleteTranslations(this.props.asset.content, index);nitpicking
| } else { | ||
| notify('Error: translation index mismatch. Cannot delete language.'); | ||
| } | ||
|
|
| {t('Here you can add one more more languages to your project, and translate the strings in each language.')} | ||
| | ||
| <em> | ||
| {t('Note: make sure your default language has a name. If it doesn\'t, you will not be able to edit your form in the form builder.')} |
There was a problem hiding this comment.
Maybe we should validate this form to not allow unnamed default language? We could use TextBox which handles errors
There was a problem hiding this comment.
"Add language" button is disabled until default language is configured :)
| .form-view__cell { | ||
| min-width: 100px; | ||
|
|
||
| &--lang-code { |
There was a problem hiding this comment.
It's just my preference, but when I try to find form-view__cell--lang-code when debugging in Dev Tools, this notation makes it so much harder
| width: 100%; | ||
| margin: 0; | ||
|
|
||
| .form-modal__item--translation-table--container { |
There was a problem hiding this comment.
Nitpicking: isn't this invalid BEM selector? I would go with .form-modal__item--translation-table-container
| } | ||
| } | ||
|
|
||
| // alertify overrides |
There was a problem hiding this comment.
all those styles below should be in jsapp/scss/components/_kobo.alertify-overrides.scss (from quick look they seem to be 100% duplicated there, but better diffcheck it)
There was a problem hiding this comment.
these were in fact duplicates plus an older version
| # run() | ||
| expect(run).toThrow('There is an unnamed translation in your form definition') | ||
| # DISABLED | ||
| # CONSIDER DELETING BEFORE MERGING |
There was a problem hiding this comment.
I would definitely be for deleting all this commented out code, but writing at least some unit tests for translations - as mentioned in one of comments above :)
There was a problem hiding this comment.
I added a pair of tests (could be more) and kept one of these here
|
I merged recent master here and added one more change in line with:
|
|
Leszek, thanks for the detailed review. If you have time to fix the many issues you’ve noticed and reorganize things (like use constants for the modal), please do. There probably should be a button in the form builder that triggers the languages. |
|
@pmusaraj I will start working on fixing what I've found :-) |
|
@magicznyleszek, re: "What is the real problem," did you see my comment above? When renaming the default language, you need to update Regarding error messages:
|
|
@jnm regarding the "Right now, I can't update b if it's set to the default, but I can update the unnamed translation" comment: the default language can currently only be edited in the form builder. we don't have an interface for updating it in the translations screens, since those strings are the base language. I do see this can be confusing for a user, especially when changing the default language. I had earlier proposed (in Flowdock) we add a confirmation modal when setting a default language, we could let the user know in that screen that strings in the default language can only be edited in the FB. |
|
@jnm I fixed updating |
Translations further fixes
|
@jnm I moved nullifying hack code to a utils function. I tested both functions with few tests (+ fixed few things thanks to those test). The problem is that both of them work on different type of data, and to get from one to other it takes some form builder code. I don't know if this is testable as a full flow. It would be easier with Cypress probably |
# Conflicts: # jsapp/js/components/formEditors.es6
Description
This PR adds UI that lets users add and update translations on a form. See screenshots below for some details. When testing, please carefully review the changes in the form builder, the PR strips out a good amount of translation-related UI there.
Screenshots
Issues
I didn't have time to do one last feature here: allow users to either:
@magicznyleszek please look into the possibility of adding this feature (might require some backend work too)
Note: the PR also reorganizes the modal forms, by putting them all in a subfolder.
Related issues
Closes #1948
Closes #1273
Closes #1175
Closes #949
Closes #1166
Closes #1144
Closes #1381
Closes #327