Skip to content
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

Dance page: Add links for translated versions of lesson plan and unplugged activity #26193

Merged
merged 6 commits into from Nov 27, 2018

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Nov 21, 2018

There are no non-English languages that are currently enabled in curriculum builder because curriculum builder will redirect all the links to point to translated versions of content for a selected language which may not exist resulting in broken links.

  • Implementation

Added logic to check the locale of the page (four character code). Based on the locale, the link is modified to point directly to the translated version of the lesson plan or unplugged activity.
Translated versions of the content are included in this PR are for the following 6 languages: Spanish (Mexico), Spanish (Spain), French, Korean, Turkish, Chinese (Traditional).

  • Before:

before-cb-link

  • After: Spanish (es & mx)

cb_link

Chinese (traditional)

screen shot 2018-11-26 at 3 39 36 pm

Korean

screen shot 2018-11-26 at 3 40 07 pm

French

screen shot 2018-11-26 at 3 40 48 pm

Turkish

screen shot 2018-11-26 at 3 41 21 pm

unplugged_link = translated_languages.include?(locale) ?
"https://curriculum.code.org/#{locale}/hoc/unplugged/4" :
"https://curriculum.code.org/hoc/unplugged/4"

Copy link
Contributor

@Erin007 Erin007 Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tiny style nits: translated_languages, lessson_link and unplugged_link should align with locale; they're currently indented too far.

@@ -30,5 +41,5 @@
.resource-description
= resource[:description]
%a{href: resource[:link]}
%button.gray-button
%button.gray-button{:id => "#{resource[:id]}"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ids are unnecessary here and in the resources hash, but I guess there's no harm in having them. Do you have an anticipated reason you'll use them later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ids are not necessary. There were added as part of an initial solution to the problem. Thanks!

@Erin007
Copy link
Contributor

Erin007 commented Nov 21, 2018

Yay! Looks like a good plan so far.

I'm concerned that only "es-mx" works right now; all of the other language codes take you to a broken page.

es-mx on curriculum:
screen shot 2018-11-21 at 8 36 19 am

all others, ko-ko shown here:
screen shot 2018-11-21 at 8 35 19 am

I don't think the problem is in how you generated the urls; I think those lessons may not be available in the other languages yet? Maybe @mrjoshida or @boatmarina know more? In the meantime, you could get this in with just "es-mx" in your translated_languages array and add in the other languages when you have confirmation that the lessons have been translated and the links work for those languages.

@ghost
Copy link

ghost commented Nov 21, 2018 via email

@Erin007
Copy link
Contributor

Erin007 commented Nov 21, 2018

When we get translations on Monday, does that mean that the links will work on Monday or that we'll have the translated content on Monday but there are still a few more steps/some more time needed to get the links working correctly? (Sorry, I'm not super familiar with the translation wizardry we do behind the scenes!)

The crux of my question is: if this PR goes in with the last deploy on Monday and we have translated curriculum pages on Monday for these languages, the links should work by Monday evening, right? I want to avoid a scenario where we link to a broken page, but if we get the timing right sounds like it could be a non-issue.

@nkiruka
Copy link
Contributor Author

nkiruka commented Nov 21, 2018

@Erin007 All good questions! I had noted this in the github bug tracking open issue yesterday awaiting response from Marina. It will be better for this PR to go out on Monday.

@@ -1,20 +1,29 @@
%link{:rel=>'stylesheet', :type=>'text/css', :href=>'/css/dance-landing/dance-teacher-resources.css'}

:ruby
locale = I18n.locale.to_s.downcase
translated_languages = ["es-mx", "es-es", "fr-fr", "ko-ko", "tr-tr", "zh-tw"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we'll have to remember to update this page whenever the docs are translated in a new locale? Is this a workaround plan, because this doesn't seem sustainable. The way I'd expect this to work is that the links on /dance always include the locale. Curriculum builder should have the logic to show you the translated version if it exists, otherwise it falls back to English.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tanyaparker Yep, this is a temporary solution. Elijah has a ToDo to determine a solution for this. (deployment.rb)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curriculum builder should have the logic to show you the translated version if it exists, otherwise it falls back to English.

Note that the fallback cannot be as simple as this, since curriculumbuilder is literally just an S3 bucket, with no ability to be aware of its own content like this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this isn't the most sustainable long-term solution. To mitigate some of the issue of having to remember when a new locale is added, is there a good place in curriculum builder or the i18n pipeline for a reminder comment that this needs to be updated?

@ghost
Copy link

ghost commented Nov 21, 2018 via email

@nkiruka
Copy link
Contributor Author

nkiruka commented Nov 21, 2018

@boatmarina The translated version of the unplugged activity is only visible for es-mx

@ghost
Copy link

ghost commented Nov 21, 2018 via email

@nkiruka
Copy link
Contributor Author

nkiruka commented Nov 21, 2018

@jakebrbell We are only seeing the translated version of the unplugged lesson for es-mx. I will follow up with you on Monday,

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we actually have translation for Spanish (Spain), just es-MX

@@ -1,20 +1,29 @@
%link{:rel=>'stylesheet', :type=>'text/css', :href=>'/css/dance-landing/dance-teacher-resources.css'}

:ruby
locale = I18n.locale.to_s.downcase
translated_languages = ["es-mx", "es-es", "fr-fr", "ko-ko", "tr-tr", "zh-tw"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curriculum builder should have the logic to show you the translated version if it exists, otherwise it falls back to English.

Note that the fallback cannot be as simple as this, since curriculumbuilder is literally just an S3 bucket, with no ability to be aware of its own content like this

@nkiruka
Copy link
Contributor Author

nkiruka commented Nov 27, 2018

@Erin007 @tanyaparker @Hamms Updated PR description and Korean locale code.

@Erin007
Copy link
Contributor

Erin007 commented Nov 27, 2018

Unplugged lesson 4 has partially translated es-es content:
unplugged

Plugged lesson 8 doesn't appear to have translated content, but the url with es-es works fine:
screen shot 2018-11-27 at 7 37 06 am

@Erin007
Copy link
Contributor

Erin007 commented Nov 27, 2018

Circle failure looks unrelated - might need to pull and merge latest staging?

@nkiruka
Copy link
Contributor Author

nkiruka commented Nov 27, 2018

@Erin007 Planned lesson will be visible today. Elijah ran a one off sync that included the lesson plan translations.

@nkiruka nkiruka merged commit dad0659 into staging Nov 27, 2018
@@ -2,7 +2,7 @@

:ruby
locale = I18n.locale.to_s.downcase
translated_languages = ["es-mx", "es-es", "fr-fr", "ko-ko", "tr-tr", "zh-tw"]
translated_languages = ["es-mx", "es-es", "ko-kr", "fr-fr", "tr-tr", "zh-tw"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this alphabetical?

@nkiruka nkiruka deleted the teacher-resources-CB-links branch December 20, 2018 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants