-
Notifications
You must be signed in to change notification settings - Fork 482
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
Translatable HoC languages string based on supported language codes #43096
Conversation
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.
Looks good! Just one comment related to improving a11y support :)
{this.props.item.language | ||
.sort((a, b) => (a > b ? 1 : -1)) | ||
.map(language => ( | ||
<div>{language}</div> |
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.
Minor nitpick, but since this is a list of items, it would be nice to use <li></li>
since those are meant for displaying lists. Using HTML elements for their specific purpose helps improve our integration with a11y tools.
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.
Ah yes thanks. Updated to use an unordered list instead.
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.
Ah yes thanks. Updated to use an unordered list instead.
We decided to not go this route actually and stick to a string. The list was considered harder to parse. Related slack thread
tutorialDetailLanguages: { | ||
overflowY: 'auto', | ||
maxHeight: '110px' | ||
}, |
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.
Is this used anywhere?
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.
We should definitely remove this. It is a remnant style of our original solution. Here is a link to the PR for clean up regarding this work.
zh-cn: "Chinese (Simplified, People's Republic of China)" | ||
zh-hans: "Chinese (Simplified) (zh-Hans)" | ||
zh-hant: "Chinese (Traditional) (zh-Hant)" | ||
zh-tw: "Chinese (Traditional, Taiwan)" |
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 slightly surprised we didn't already translate this list; any chance we did?
Also, is it possible to "indent" these under a parent tag to group them? Or otherwise add a prefix explaining their context?
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.
Also, do we actually need to do this? At least for our language dropdown across the site, we've been alright with showing each language in its own 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 can't find a thread in regards to our decision making here, but I'll pull in @dju90 here to fill in any context if I miss any. We received feedback from Jake on International that it would be more beneficial to have the languages in these fields to all be in the currently viewing locale.
I'm aware of the cdo_languages
table in our Pegasus DB, but that doesn't seem to help at all in our goal to create translations for all languages outside of English. I didn't see any other translation files that would help in our case here either (ex. user viewing in Hebrew reading the translated version of Persian).
In regards to grouping these, I think it makes sense to add a prefix like language_code_
or something. I need to check with Jorge if they've made significant progress on getting translations in other languages for these. If not I can add that change easily.
@@ -128,7 +128,7 @@ social: | |||
- tutorial[:name] = hoc_s(prefix + "name") | |||
- tutorial[:shortdescription] = hoc_s(prefix + "shortdescription") | |||
- tutorial[:longdescription] = hoc_s(prefix + "longdescription") | |||
- tutorial[:language] = hoc_s(prefix + "language") | |||
- tutorial[:language] = hoc_language(tutorial[:languages_supported]) |
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 think this change also needs to be made for https://code.org/learn, here.
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 fact, production https://code.org/learn currently has blank language fields due to the strings being removed. Making the same change there should fix it.
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.
Addressed this in this PR as well.
@@ -119,6 +119,15 @@ def hoc_get_locale_code | |||
Languages.get_hoc_locale_by_unique_language(@language) | |||
end | |||
|
|||
# hourofcode.com calls this to translate tutorial's languages attribute |
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.
This should also mention code.org
when that's fixed, and we could specifically mention that it's used on /learn
on each of those sites.
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.
Good callout. Fix in this PR.
|
||
# Convert language codes to array and get the translated string | ||
language_codes = lang_codes_str.split(',') | ||
language_codes.map {|code| hoc_s(code.downcase)}.select {|code| code}.join ", " |
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.
Purely out of curiosity, is a comma-separated list the appropriate output for all of our destination locales? (I guess we can confirm by looking at existing translations of the list.)
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.
This was a concern we considered. I have a lack of experience of what obstacles a comma separated list has for certain languages, so I deferred to the team (as well as our input here). The conclusion I believe was to go with this format being it wasn't too detrimental.
If this removes the need for the |
What: Setting the tutorial's languages attribute displayed on our
tutorialExplorer
based on the supported language codes instead of a unique string for each tutorial. A user will see all of the languages in their current viewing locale.Why: This allows each language to be individually translated by each locale and remove the overhead of having to individually translate each tutorial's language string.
Links
jira ticket: FND-1636
Testing story
Tested locally and confirmed English is working as expected. Made dummy translations for another locale and those were shown as expected, with the fallback of English showing when translations didn't exist.
Follow-up work
Next i18n sync will post these new translations to Crowdin
PR Checklist: