-
Notifications
You must be signed in to change notification settings - Fork 479
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
LATAM HOC promo video on hourofcode.com homepage #25039
Conversation
if (videoStarted) | ||
return; | ||
|
||
$("<iframe allowfullscreen frameborder='0' src='https://www.youtube-nocookie.com/embed/OpgWbXthMlQ?iv_load_policy=3&rel=0&autohide=1&showinfo=0&autoplay=1&cc_load_policy=1' style='position:absolute; top: 0; left: 0; width: 100%; height: 100%; background-color:black;'></iframe>").appendTo("#videodiv"); |
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 still waiting on the video code - right now this plays a Finding Nemo clip 🐠. Double check that I update when the video is ready!
I did this in a way that duplicates a big chunk of code for a few reasons: 1.) It was fastest 2.) I knew it wouldn't break the current video for other languages 3.) It's temporary, so keeping everything separate rather than parameterized makes it easier to delete later. 4.) It was fastest. I'm open to other suggestions if we think the duplication is too gross to live with. |
clearInterval(timerId); | ||
} | ||
$(function() { | ||
$('.highlight-item').mouseover(function() { |
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.
Do we expect the caption to be available on touch devices in some way?
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.
How come this part has to be repeated even though it's exactly the same as the original video and it's outside of the startLATAMVideo() function? (I may just not understand Javascript)
$(function() {
$('.highlight-item').mouseover(function() {
var caption = $(this).find('.highlight-caption');
caption.fadeIn();
});
$('.highlight-item').mouseleave(function() {
var caption = $(this).find('.highlight-caption');
caption.fadeOut();
});
});
%img{src: "/images/play-button.png", width: "60", onclick: "return startVideo();", style: "cursor: pointer;"} | ||
-# If language is Espanol(Mexico), show alternate promo video | ||
-# for LATM Hour of Code. | ||
- unless @language == "la" |
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.
Deliberately not doing an else
to complement the new if
?
This seems fine if it's truly temporary code. But if there's a chance that this kind of change becomes an annual occurrence we should think about how to drive it by DCDO or some other kind of configuration rather than committing code swaps repeatedly, of course. |
%div.watch-video-small{onclick: "return startLATAMVideo();"} | ||
=hoc_s(:front_watch_regular_video) | ||
%img{src: "/images/play-button.png", width: "25", onclick: "return startLATAMVideo();", style: "cursor: pointer;"} | ||
- else |
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.
using all three of unless
, if
, and else
seems redundant, unless I'm misunderstanding something; won't this render the video twice for non-LA?
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.
Done!
@@ -14,6 +14,10 @@ social: | |||
-# whenever we change the hoc_mode, to make sure we're still testing what we'll see on production. | |||
-hoc_mode = DCDO.get("hoc_mode", CDO.default_hoc_mode) | |||
|
|||
-# Spanish(Mexico), Spanish(Spain), and Portuguese(Brazil) |
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 it intentional that we're showing Latin American content for a European locale?
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.
Yes, per a request from Suky/Hadi we want all Spanish locales to see this video.
-# promo video for LATM Hour of Code. | ||
- if latam_language_codes.include?(@language) && show_latam_videos | ||
%h1.front-header-banner.watch-video=hoc_s(:front_watch_regular_video) | ||
%img{src: "/images/play-button.png", width: "60", onclick: "return startLATMVideo();", style: "cursor: pointer;"} |
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.
Typo startLATMVideo
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! Good catch - I did that with the branch names too 🙃
%h1.front-header-banner.watch-video=hoc_s(:front_watch_regular_video) | ||
%img{src: "/images/play-button.png", width: "60", onclick: "return startVideo();", style: "cursor: pointer;"} | ||
-# If language Spanish or Brazilian Portuguese, show alternate | ||
-# promo video for LATM Hour of Code. |
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.
Typo: LATM
I don't know if I would say that this is temporary. Even if we take it down for a bit while we show some other videos for other hoc things we're announcing this year, we'll want to have this video be the LATAM video for the rest of the year. |
Can you explain why we already have two places to start the video? What's the difference between the |
I'm going to get this in as is today, and then re-visit for a more long term refactor to deduplicate and more gracefully handle hiding/showing these videos. |
Oh, gotcha. Thanks for explaining! |
Users viewing the site in Spanish(Mexico) "la", Spanish(Spain) "es", or Portuguese(Brazil) "pt", will see a different promotional video on the Hour of Code homepage to promote Latin American Hour of Code. Users in all other languages are not affected.
Similar to #25023