fix: translation on select problem type #1798
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @igobranco! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
The select problem type and advanced problem types didn't function correctly with i18n/translation.
igobranco
force-pushed
the
igobranco/fix-translation-select-problem-type
branch
from
f790b02 to
173d73d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1798 +/- ##
==========================================
+ Coverage 93.50% 93.51% +0.01%
==========================================
Files 1128 1131 +3
Lines 22925 22970 +45
Branches 4953 4959 +6
==========================================
+ Hits 21436 21481 +45
Misses 1413 1413
Partials 76 76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Extract from problem.ts, its types and its messages. The messages uses the types definition. The problem file depends on both and reexport everything so we don't have to change all project.
| @@ -54,25 +55,29 @@ const AdvanceTypeSelect: React.FC<Props> = ({ | |||
| className="px-4" | |||
| > | |||
| {Object.entries(AdvanceProblems).map(([type, data]) => { | |||
| if (data.status !== '') { | |||
| if (data.status !== AdvanceProblemsStatus.empty) { | |||
There was a problem hiding this comment.
This feels odd to me for a couple reasons:
- Internationalizing an empty string seems unnecessary
- We aren't using
intlhere, so wouldn't this end up checking for the object instead of the string?
I'd think sticking to '' would be simpler and continue to work. Did you run into something that made that not work?
There was a problem hiding this comment.
yes, it is kind of strange, but the status of AdvanceProblems is now a AdvanceProblemsStatus.
| return ( | ||
| <ActionRow className="border-primary-100 border-bottom m-0 py-3 w-100" key={type}> | ||
| <Form.Radio id={type} value={type}> | ||
| {intl.formatMessage(messages.advanceProblemTypeLabel, { problemType: data.title })} | ||
| {intl.formatMessage(messages.advanceProblemTypeLabel, { | ||
| problemType: intl.formatMessage(data.titleMessage), |
There was a problem hiding this comment.
Could you elaborate on the title vs titleMessage stuff? I see down in problem.ts you've added titleMessage and set title to use the defaultMessage.
There was a problem hiding this comment.
The idea of titleMessage was to prevent having to create further code refactor on the references of the problem.ts file.
But still kind of using the existing design of Object Oriented idea of the ProblemTypes and AdvanceProblems.
And extracting the i18n to a proper defineMessages so the formatjs can detect them.
So, when you want to present them to the user, I just replace the title with titleMessage on a intl.formatMessage(...).
| </Form.Radio> | ||
| <ActionRow.Spacer /> | ||
| <OverlayTrigger | ||
| placement="right" | ||
| overlay={( | ||
| <Tooltip id={`tooltip-adv-${type}`}> | ||
| <div className="text-left"> | ||
| {intl.formatMessage(messages.supportStatusTooltipMessage, { supportStatus: data.status.replace(' ', '_') })} | ||
| {intl.formatMessage(messages.supportStatusTooltipMessage, { supportStatus: data.status.defaultMessage.replace(' ', '_') })} |
There was a problem hiding this comment.
Why is this using the default message instead of an internationalized version?
There was a problem hiding this comment.
Because of the supportStatusTooltipMessage is a select, so to decide which one should be shown the key is the default value one i18n or Not_supported or Provisional. I prefer not to change too much the existing code.
The replace(' ', '_') probably is because the select from i18n don't allow the key to have whitespace.
When I add the translation to pt-pt locale on src/i18n/messages/frontend-app-course-authoring/pt_PT.json:
"authoring.problemEditor.advanceProblem.supportStatus.tooltipMessage": "{supportStatus, select, Provisional {As ferramentas suportadas provisoriamente podem não ter a robustez de funcionalidade que os seus cursos exigem. O edX não tem controlo sobre a qualidade do software ou do conteúdo que pode ser fornecido utilizando estas ferramentas. Teste estas ferramentas cuidadosamente antes de as utilizar no seu curso, especialmente em secções com classificação. A documentação completa pode não estar disponível para as ferramentas suportadas provisoriamente, ou a documentação pode estar disponível a partir de outras fontes que não a edX.} Not_supported {As ferramentas sem suporte não são mantidas pela edX e podem ser descontinuadas no futuro. Não são recomendadas para utilização em cursos devido à não conformidade com um ou mais dos requisitos básicos, tais como testes, acessibilidade, internacionalização e documentação.} other { } }",It shows correctly has:
The select problem type and advanced problem types didn't function correctly with i18n/translation.
Description
This PR fixes the translations/i18n of the "Select problem type"; and following screen "Advanced problem type" selection; and the following screen that shows a description of the problem.
Next some screenshots with highlighted section fixed:
Supporting information
Any. Just private internal issues on NAU platform.
Testing instructions
Add to your language json file, example for pt-pt locale
src/i18n/messages/frontend-app-course-authoring/pt_PT.json:Other information