-
-
Notifications
You must be signed in to change notification settings - Fork 181
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
[TASK-624] Allow users to select between translated labels and XML names for displayed form questions in NLP UI #4933
[TASK-624] Allow users to select between translated labels and XML names for displayed form questions in NLP UI #4933
Conversation
Please enter the commit message for your changes. Lines starting
…dons-task-624-change-displayed-language
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.module.scss
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
and some basic PR changes Conflicts: jsapp/js/components/processing/sidebar/sidebarDisplaySettings.module.scss
jsapp/js/components/processing/sidebar/sidebarLabelsSettings.component.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarLabelsSettings.component.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarLabelsSettings.component.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarLabelsSettings.component.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarLabelsSettings.component.tsx
Outdated
Show resolved
Hide resolved
} | ||
}); | ||
} else { | ||
languagesList.push({label: t('Default'), value: 'default'}); |
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.
Could you make it so it works similarly to "Data → Table → Display options" modal? I.e. that you will see the "default" option as being selected? I'm guessing you would need to return 'default'
in getInitialDisplayedLanguage
instead of empty string. Or change the value: 'default'
to value: ''
getDisplayedLanguagesList(): KoboSelectOption[] { | ||
const languagesList = []; | ||
|
||
languagesList.push({label: t('XML names'), value: 'xml_names'}); |
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.
In "Data → Table → Display options" modal, if you have a project with no languages being defined, you will see two options: "XML Values" and "Labels" - let's stick to the same naming, so it's clear these are the same.
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.
Tried making the changes for this one and above. Not 100% I understand so lmk if I did it right
<Button | ||
size='m' | ||
type='bare' | ||
label={t('Change question language')} |
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'm not fond of this label. And as I was trying to think about better name, like "Labels settings", "Data settings", I started thinking about moving this into the "Display settings" modal after all… Sorry! Will ask Tessa in public topic about her two cents.
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 decision on Zulip topic is to put labels settings inside display settings modal, sorry!
jsapp/js/components/processing/sidebar/sidebarLabelsSettings.component.tsx
Outdated
Show resolved
Hide resolved
TODO: move label display back to display settings
Conflicts: jsapp/js/components/processing/sidebar/processingSidebar.tsx
Checklist
Description
Adds a selector to the NLP UI to allow switching question label languages. The places that would be affected by such a change are:
Notes
Ended up having to pass
asset
instead ofassetContent
in some places due to needing the entireAssetResponse
in order to usegetLanguageIndex
. Tried to make these changes minimal but maybe better to just change all cases to useasset
instead of sometimes both.Related issues
Task 624 of NLP improvements