Conversation
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 2 comments.
Reviewable status: 0 of 2 files reviewed, all discussions resolved.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 197 at r1 (raw file):
[placeholder]="t('bt_project_placeholder')" [projects]="availableProjects" [resources]="availableResources"
Resources should never have been an option when selecting the back translation
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 81 at r1 (raw file):
}) export class DraftOnboardingFormComponent extends DataLoadingComponent implements OnInit { signupForm = new FormGroup({
Nothing about the signup form was changed; the initialization was just moved.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3680 +/- ##
==========================================
- Coverage 81.84% 81.82% -0.03%
==========================================
Files 619 619
Lines 38608 38619 +11
Branches 6290 6293 +3
==========================================
+ Hits 31600 31601 +1
- Misses 6047 6057 +10
Partials 961 961 ☔ View full report in Codecov by Sentry. |
RaymondLuong3
left a comment
There was a problem hiding this comment.
This makes sense for the serval admins to see the language name in English, but I'm not sure how a non-English speaker would interpret that field? Most likely they would put the local name of the language which is less useful for us. Is it necessary to ask the user for the name of the back translation when we can calculate it ourselves after the form is submitted? I would be in favour of removing that field completely.
@RaymondLuong3 reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 339 at r1 (raw file):
// Attempt to get the English name of the language from the browser const englishName = new Intl.DisplayNames(['en'], { type: 'language' }).of(selectedProject.languageTag);
We have similar logic in i18n.service.ts under getLanguageDisplayName(). Could you adapt what is already in i18n.service to extract the english name from the language code?
Code quote:
const englishName = new Intl.DisplayNames(['en'], { type: 'language' }).of(selectedProject.languageTag)04dc7f2 to
ebb1d17
Compare
Nateowami
left a comment
There was a problem hiding this comment.
The EITL team has found that the back translation language code is often incorrect and made the deliberate decision to keep these fields. The form is being submitted to the EITL team, so information should generally be in English.
@Nateowami made 2 comments.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 339 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
We have similar logic in
i18n.service.tsundergetLanguageDisplayName().Could you adapt what is already in i18n.service to extract the english name from the language code?
That's a good point. I initially didn't do that because the I18nService is focused on providing localizations, and this is focused on getting the language name in English, which isn't a localization task.
However, looking at the implementation in the I18n service, I see we've worked around an error in Firefox, so it probably really does make sense to re-use the same method.
RaymondLuong3
left a comment
There was a problem hiding this comment.
Aha, thanks for that explanation. I guess that is not unexpected. This should help make that process better.
@RaymondLuong3 reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Nateowami).
ebb1d17 to
1b9d6c8
Compare
This change is