Bug 797379 State of Mozilla #457

Merged
merged 29 commits into from Nov 14, 2012

6 participants

@craigcook
Mozilla member

We're still awaiting the PDF files. We can add them at the last minute, or the fallback plan (if we don't get them by Monday) is to comment out the links at the bottom and add the PDFs as soon as they're available.

The javascript file just kept snowballing so I know it's not as clean as it could be (functions should probably be declared as variables with a namespace, I've got a lot of redundancies and ifs, etc).

craigcook and others added some commits Oct 26, 2012
@craigcook craigcook Squashed commit of the following:
commit ac047201a00708f117cf19d4711f81aab7a63cf1
Merge: 6f42c1a ca7448d
Author: Craig Cook <craigcook@gmail.com>
Date:   Fri Oct 26 10:38:46 2012 -0700

    Merge branch 'bug-797379-state-of-mozilla-2011' of github.com:craigcook/bedrock into bug-797379-state-of-mozilla-2011

commit 6f42c1a313eafd68a6451351233e54a71afb19fe
Author: Craig Cook <craigcook@gmail.com>
Date:   Fri Oct 26 10:24:26 2012 -0700

    Progress

commit bb46674515d85bb7660edcf80d807e26bfc59019
Author: Craig Cook <craigcook@gmail.com>
Date:   Thu Oct 25 18:01:19 2012 -0700

    Further progress

commit e07ac24e3eba0e53879b4556a177e902ce13d44b
Author: Craig Cook <craigcook@gmail.com>
Date:   Wed Oct 24 15:41:55 2012 -0700

    Progress. A few more sections, script tweaks, some new strings for l10n that I missed.

commit a4a919e5dcc3b6cb8a78b10f0013d0d171ebdb23
Author: Craig Cook <craigcook@gmail.com>
Date:   Tue Oct 23 12:12:09 2012 -0700

    i18n improvements

commit 0b852004fe1dbdbae4b79e209fd2e254ea6470fc
Author: Craig Cook <craigcook@gmail.com>
Date:   Wed Oct 17 16:42:56 2012 -0700

    Added FAQ page, giving l10n a head start

commit b028fb8fbabc57c96513820f6e25aac47df4215e
Author: Craig Cook <craigcook@gmail.com>
Date:   Fri Oct 12 00:58:32 2012 -0700

    First commit, saving progress

commit ca7448d5b98cb519cc860f43b818e4c7e5242603
Author: Craig Cook <craigcook@gmail.com>
Date:   Fri Oct 26 10:24:26 2012 -0700

    Progress

commit e4c22ca09ef4d423c497a1ad9b6fc51e5d915789
Author: Craig Cook <craigcook@gmail.com>
Date:   Thu Oct 25 18:01:19 2012 -0700

    Further progress

commit 55fcd4fe003d86bdfdbf5b514b8fa1db8dfff913
Author: Craig Cook <craigcook@gmail.com>
Date:   Wed Oct 24 15:41:55 2012 -0700

    Progress. A few more sections, script tweaks, some new strings for l10n that I missed.

commit 7216575517186d6efe8abad999691e15b174f183
Author: Craig Cook <craigcook@gmail.com>
Date:   Tue Oct 23 12:12:09 2012 -0700

    i18n improvements

commit 8bef1fb0e8476c8fb49be0e9493414dcde0789ea
Merge: f7bb597 ffd96df
Author: Craig Cook <craigcook@gmail.com>
Date:   Tue Oct 23 08:52:43 2012 -0700

    Merge branch 'bug-797379-state-of-mozilla-2011' of github.com:craigcook/bedrock into bug-797379-state-of-mozilla-2011

    Conflicts:
    	apps/foundation/templates/foundation/_country_select.html

commit f7bb597e2f0eb1a30e0cd0cc95e321b128e4ce4a
Author: Craig Cook <craigcook@gmail.com>
Date:   Wed Oct 17 16:42:56 2012 -0700

    Added FAQ page, giving l10n a head start

commit 0d330351766000b462813695828a5b7fe3fc3b62
Author: Craig Cook <craigcook@gmail.com>
Date:   Fri Oct 12 00:58:32 2012 -0700

    First commit, saving progress

commit ffd96df89f166f45e785813471662db3c77482ab
Author: Craig Cook <craigcook@gmail.com>
Date:   Wed Oct 17 16:42:56 2012 -0700

    Added FAQ page, giving l10n a head start

commit c43acc0202746b8b5507dbc6710d0e29512ad880
Author: Craig Cook <craigcook@gmail.com>
Date:   Fri Oct 12 00:58:32 2012 -0700

    First commit, saving progress
f22d212
@pascalchevrel pascalchevrel Bug 806939 - FAQ link not working because it lacks a function call 19776bc
@craigcook craigcook Latest updates
* More content blocks
* Removed wordmark
* Nixed text modals
* Links open new windows
* Fixed up non-JS styling
* Better keyboard access
* Fixed FAQ link
* Misc tidying
6c0272c
@craigcook craigcook Merge branch 'bug-797379-state-of-mozilla' of github.com:mozilla/bedr…
…ock into bug-797379-state-of-mozilla

Conflicts:
	apps/foundation/templates/foundation/annualreport/2011.html
fe516ba
@craigcook craigcook Fixed some typos (see bug 788871) 9dbcbdd
@rik rik Merge branch 'master' into bug-797379-state-of-mozilla 264c713
@craigcook craigcook Updates to State of Mozilla
Navigation
Contributor stories (some new strings here, sorry Pascal)
Sustainability section
ff4af7b
@craigcook craigcook Merge branch 'bug-797379-state-of-mozilla' of github.com:mozilla/bedr…
…ock into bug-797379-state-of-mozilla
319aaab
@pascalchevrel pascalchevrel Bug 807301 - state of mozilla: reuse localized strings in FAQ page fr…
…om main page
2e5ed18
@craigcook craigcook Fixed JS error for FAQ page
Script was trying to change attributes of an element that doesn't exist on the page. Fixed by wrapping it in an if.
8246e76
@craigcook craigcook Merge branch 'bug-797379-state-of-mozilla' of github.com:mozilla/bedr…
…ock into bug-797379-state-of-mozilla
b91e80f
@pmac pmac Merge branch 'master' into bug-797379-state-of-mozilla e7a2e40
@craigcook craigcook Updates
* Added section breaks/headings
* Swapped Flicks and Collusion
* Responsified for tablet size

Fix bug 806936 - adding partner logos to the moz.org page

Bug 805679 Add wordmark to Firefox OS page in style guide

Typo in persona/developer-faq.html, misplaced ')'

Correctly install gettext into Jinja2. Fix bug 808580.

Bug 807301 - state of mozilla: reuse localized strings in FAQ page from main page

Bug 808539 - state of mozilla: specify lang file to use for the main page

Bug 809349 - Make <title> and <h1> tags localizable on State of Mozilla

updates for Bug 809349

Progress on responsification
Still working on menu and mobile landscape
788db23
@craigcook craigcook Merge branch 'bug-797379-state-of-mozilla' of github.com:mozilla/bedr…
…ock into bug-797379-state-of-mozilla
86b7003
@craigcook craigcook Merge branch 'master' of github.com:mozilla/bedrock into bug-797379-s…
…tate-of-mozilla

Conflicts:
	settings/base.py
6a0facc
@craigcook craigcook Layout for mobile landscape, some script cleanup/fixes 27ef23d
@craigcook craigcook Responsive navigation
Still a little twitchy, but it works
325d130
@craigcook craigcook Updated financial figures e9f037f
@craigcook craigcook More checks to prevent errors
This ragtag jumble of scripts is kind of a mess, could use a serious refactor but time is short. Still working on a better mobile-friendly menu solution.
334bd91
@craigcook craigcook Added print styles, misc code tidying 6baca27
@pascalchevrel pascalchevrel make 'SOPA / PIPA & ACTA' a localizable string (& sign is not universal) 9506e19
@craigcook craigcook Merge pull request #458 from pascalchevrel/bug-797379-state-of-mozilla
Make 'SOPA / PIPA & ACTA' a localizable string (& sign is not universal)
bcda543
@pmclanahan pmclanahan commented on the diff Nov 12, 2012
...oundation/templates/foundation/annualreport/2011.html
+ <div class="overlay-wrap">
+ <aside class="pull">
+ <blockquote>
+ <p>
+ {% trans %}
+ Just like we did on the desktop, Mozilla is setting out to
+ ensure that the mobile Web is full of freedom, choice &
+ opportunity.
+ {% endtrans %}
+ </p>
+ </blockquote>
+ </aside>
+
+ <div class="overlay">
+ <p>
+ {% trans
@pmclanahan
Mozilla member
pmclanahan added a line comment Nov 12, 2012

Why 2 lines for this tag?

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

Just formatting. When there are multiple variables I like putting each on its own line but that's not necessary for singletons. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on the diff Nov 12, 2012
apps/foundation/urls.py
@@ -0,0 +1,10 @@
+from django.conf.urls.defaults import *
+from mozorg.util import page
+
+
+urlpatterns = patterns('',
+
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

Any reason for this space? </sillynit>

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

None at all. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on the diff Nov 12, 2012
media/js/annual2011.js
@@ -0,0 +1,398 @@
+(function() {
+
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

'use strict' is missing, you should run this file through JSHint: http://www.jshint.com/

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

Added.

@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

The file still doesn't pass JSHint though. :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
@@ -0,0 +1,398 @@
+(function() {
+
+ var $window = $(window);
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

JS Styleguide says you shouldn't line up variable assignments: http://mozweb.readthedocs.org/en/latest/js-style.html#js-style

There's several other violations that I'm not going to keep pointing out, like 4-space indents and if then else lineup issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+
+ $window.resize(function() {
+ clearTimeout(this.id);
+ this.id = setTimeout(doneResizing, 500);
+ });
+
+ function doneResizing() {
+ navHeight = $nav.height() + 30;
+ if ($window.width() >= 768) {
+ wideMode = true;
+ if ( $("#story-slider-clone").length === 0 ) {
+ setupCarousel();
+ $("#video-stage").show();
+ }
+ }
+ else if ($window.width() < 768) {
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

Condition is unnecessary, as you already confirmed this is true when the first condition failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+ });
+ };
+
+ // Set up waypoints for scrolling; update nav for each section
+ // Uses jQuery Waypoints http://imakewebthings.com/jquery-waypoints/
+ $('#welcome').waypoint(function(event, direction) {
+ if(fixed) {
+ $nav.attr('class', 'fixed welcome');
+ $nav.find("li").removeClass();
+ $("#nav-welcome").addClass("current");
+ }
+ },{
+ offset: navHeight
+ });
+
+ $('#mobilized').waypoint(function(event, direction) {
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

Instead of repeating this callback three times and replacing a few values, you should make a function that takes in the values you want to replace and returns this callback, referencing those values. Something like:

function waypointCallback(current, previous) {
    return function(event, direction) {
        if (fixed) {
            if (direction == 'down') {
                $nav.attr('class', 'fixed ' + current);

... and so on. Then,

$('#mobilized').waypoint(waypointCallback('mobilized', 'welcome'));
$('#action').waypoint(waypointCallback('action', 'mobilized'));
// and so on...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+ $nav.attr('class', 'fixed sustainability');
+ $nav.find("li").removeClass();
+ $("#nav-sustainability").addClass("current");
+ }
+ else {
+ $nav.attr('class', 'fixed community');
+ $nav.find("li").removeClass();
+ $("#nav-community").addClass("current");
+ }
+ }
+ },{
+ offset: navHeight
+ });
+
+ // Check for an adjusted scrollbar every 100ms.
+ setInterval(adjustScrollbar, 100);
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

This should probably be right below adjustScrollbar to make it clearer, or even just pass the function in directly if you're not using it anywhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+
+ // Bind scrolling to linked element.
+ $window.on('click', '#page-nav a[href^="#"]', function(e) {
+ e.preventDefault();
+ // Extract the target element's ID from the link's href.
+ var elem = $(this).attr("href").replace( /.*?(#.*)/g, "$1" );
+ $('html, body').animate({
+ scrollTop: $(elem).offset().top - 35
+ }, 1000, function() {
+ $(elem).attr('tabindex','100').focus().removeAttr('tabindex');
+ if (!wideMode) { $nav.removeAttr("style"); }
+ });
+ });
+
+ // Load links in new tab/window
+ $("a[rel='external']").click( function(e) {
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

Why not use target="_blank"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+ $(elem).attr('tabindex','100').focus().removeAttr('tabindex');
+ if (!wideMode) { $nav.removeAttr("style"); }
+ });
+ });
+
+ // Load links in new tab/window
+ $("a[rel='external']").click( function(e) {
+ e.preventDefault();
+ window.open(this.href);
+ });
+
+ // Load videos in a full-page modal
+ $("a.video-play").click( function(e) {
+ e.preventDefault();
+ var origin = $(this);
+ var video = $(this).attr("data-video-source");
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

You should be able to use $.data instead:

var $origin = $(this);
var video = $origin.data('videoSource')
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+ });
+ });
+
+ // Load links in new tab/window
+ $("a[rel='external']").click( function(e) {
+ e.preventDefault();
+ window.open(this.href);
+ });
+
+ // Load videos in a full-page modal
+ $("a.video-play").click( function(e) {
+ e.preventDefault();
+ var origin = $(this);
+ var video = $(this).attr("data-video-source");
+ var poster = $(this).children("img").attr("src");
+ $('body').addClass("noscroll").append('<div id="fill"><div id="inner"><video id="video" poster="'+poster+'" controls autoplay></video></div></div>');
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

You should pull out adding the modal to the page into a seperate function and make it return a jQuery object of the inner div to allow for inserting content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+ e.preventDefault();
+ window.open(this.href);
+ });
+
+ // Load videos in a full-page modal
+ $("a.video-play").click( function(e) {
+ e.preventDefault();
+ var origin = $(this);
+ var video = $(this).attr("data-video-source");
+ var poster = $(this).children("img").attr("src");
+ $('body').addClass("noscroll").append('<div id="fill"><div id="inner"><video id="video" poster="'+poster+'" controls autoplay></video></div></div>');
+ $("#video").append(
+ '<source src="'+video+'.webm" type="video/webm">'
+ +'<source src="'+video+'.mp4" type="video/mp4">'
+ ).focus();
+ closeModal();
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

I'm confused, doesn't this close the modal as soon as it's opened?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+ $("#video")[0].pause();
+ $("#fill").remove();
+ }
+ $('body').addClass("noscroll").append('<div id="fill"><div id="inner"><video id="video" poster="'+poster+'" controls></video></div></div>');
+ $("#video").append(
+ '<source src="'+video+'" type="video/webm">'
+ ).focus()[0].play();
+ $("#inner").append('<p class="desc">'+desc+'</p>');
+ closeModal();
+ }
+ });
+ };
+
+
+ // Remove the full-page modal
+ function closeModal() {
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

This doesn't close the modal, it binds the events for closing it. This should be part of the createModal function. Closing the video and removing the fill should be in callbacks passed to createModal:

function createModal(content, onClose) {
    // Create the modal or select and clear existing modal html
    $inner.append(content);

    // Inside the event handlers...
    onClose();
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 12, 2012
media/js/annual2011.js
+
+
+ // Remove the full-page modal
+ function closeModal() {
+ $("#done").clone().appendTo("#inner");
+ $("#fill #done").bind('click', function() {
+ $("#video")[0].pause();
+ $("#fill").remove();
+ $("body").removeClass("noscroll");
+ });
+
+ $("#fill").bind('keyup', function(e) {
+ if (e.keyCode == 27) { // esc
+ $("#fill").remove();
+ $("body").removeClass("noscroll");
+ origin.focus();
@Osmose
Mozilla member
Osmose added a line comment Nov 12, 2012

origin isn't defined here IIRC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
craigcook added some commits Nov 13, 2012
@craigcook craigcook Merge pull request #460 from pascalchevrel/bug-797379-V2
css fixes for l10n
3b85658
@craigcook craigcook Updates from review feedback
* Bunch of format/style fixes
* Tidying up some JS, moving repetative stuff into functions
31be25f
@rik rik commented on an outdated diff Nov 13, 2012
@@ -27,6 +27,7 @@
(r'', include('mozorg.urls')),
(r'', include('privacy.urls')),
(r'', include('research.urls')),
+ (r'', include('foundation.urls')),
@rik
rik added a line comment Nov 13, 2012

Can you change this to r'^foundation'? And remove the foundation prefix in apps/foundation/urls.py. This will avoid a bit of namespace pollution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rik

I'd like it if @sgarrity could take a look at the LESS changes.

@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ setStage();
+ $("#video-stage").show();
+ } else {
+ $("#video-stage").hide();
+ }
+
+ $window.resize(function() {
+ clearTimeout(this.id);
+ this.id = setTimeout(doneResizing, 500);
+ });
+
+ function doneResizing() {
+ navHeight = $nav.height() + 30;
+ if ($window.width() >= 768) {
+ wideMode = true;
+ if ( $("#story-slider-clone").length === 0 ) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Extra spaces here, around the condition inside the parens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+
+ $window.resize(function() {
+ clearTimeout(this.id);
+ this.id = setTimeout(doneResizing, 500);
+ });
+
+ function doneResizing() {
+ navHeight = $nav.height() + 30;
+ if ($window.width() >= 768) {
+ wideMode = true;
+ if ( $("#story-slider-clone").length === 0 ) {
+ setupCarousel();
+ $("#video-stage").show();
+ }
+ }
+ else {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Extra newline, else should be on the same line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ // This is for smaller viewports that don't get the slider
+ function removeCarousel() {
+ $("#story-slider-clone").parents(".jcarousel-container").remove();
+ if ($("#story-vid")[0].paused == false) {
+ $("#story-vid")[0].pause();
+ }
+ setupThumbnails();
+ $sliderPrime.show();
+ }
+
+
+ // Contributor story thumbnails play the corresponding video
+ function setupThumbnails() {
+ $("a.contributor").click(function(e) {
+ e.preventDefault();
+ var video = $(this).attr("href");
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Fix the alignment on these, please.

@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Set $origin once up here and use it throughout the function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ }
+ setupThumbnails();
+ $sliderPrime.show();
+ }
+
+
+ // Contributor story thumbnails play the corresponding video
+ function setupThumbnails() {
+ $("a.contributor").click(function(e) {
+ e.preventDefault();
+ var video = $(this).attr("href");
+ var poster = $(this).attr("data-poster");
+ var desc = $(this).find(".note").html();
+
+ if (wideMode) {
+ if ($("#story-vid")[0].paused == false) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Does this really need a check? You should just always attempt to pause it, if it's already paused it'll do nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose and 1 other commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ function setupThumbnails() {
+ $("a.contributor").click(function(e) {
+ e.preventDefault();
+ var video = $(this).attr("href");
+ var poster = $(this).attr("data-poster");
+ var desc = $(this).find(".note").html();
+
+ if (wideMode) {
+ if ($("#story-vid")[0].paused == false) {
+ $("#story-vid")[0].pause();
+ }
+ if ( $("span.btn-play") ) {
+ $("span.btn-play").remove();
+ }
+ $("#video-stage figcaption").fadeOut('fast', function(){ $(this).html(desc).fadeIn('fast'); });
+ $("#story-vid").attr('poster', poster).attr('controls','controls').attr('src', video)[0].play();
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Can't you set controls on the element itself and not here? IIRC they don't show up until the video starts playing.

Also, from my testing, changing the poster doesn't seem to do anything since you call play immediately, which gets rid of the poster and shows the loading icon. Unless behavior in other browsers is different, you could probably get away with removing the other posters and data attributes and just use the first one.

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

If controls are enabled when the page loads they're visible by default, which is kind of ugly and doesn't match the design. Adding the controls attribute once we initiate play means they're not visible until we're ready for them.

And true, changing the poster doesn't seem to do anything, I just wanted the poster to match the video because I'm anal like that.

@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Does changing the poster add an extra http request though?

EDIT: My quick testing indicates that it does! :(

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

Oh fine then, I'll just bury my pain deep down and let the poster be wrong.

Incidentally, Chrome does show the briefest flash of the new poster before the video starts playing, so removing the poster change prevents that flash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ $("#story-vid")[0].pause();
+ }
+ if ( $("span.btn-play") ) {
+ $("span.btn-play").remove();
+ }
+ $("#video-stage figcaption").fadeOut('fast', function(){ $(this).html(desc).fadeIn('fast'); });
+ $("#story-vid").attr('poster', poster).attr('controls','controls').attr('src', video)[0].play();
+ $('html, body').animate({
+ scrollTop: $("#video-stage").offset().top - 80
+ }, 200, function() {
+ $("#story-vid").focus();
+ });
+ } else {
+ e.preventDefault();
+ var $origin = $(this);
+ var content =
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Formatting of this could be a bit better. I think JSHint throws an error about this. You should move this string to the same line as var content and surround the string with parenthesis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ }, 200, function() {
+ $("#story-vid").focus();
+ });
+ } else {
+ e.preventDefault();
+ var $origin = $(this);
+ var content =
+ '<video id="video" poster="'+poster+'" src="'+video+'" controls autoplay type="video/webm">'
+ +'<p class="desc">'+desc+'</p>';
+ videoModal($origin, content);
+ }
+ });
+ }
+
+ function videoModal(origin, content) {
+ if ($("#fill").length > 0) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Lots of jQuery functions that could be pulled into variables here, like $fill and $video.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ scrollTop: $("#video-stage").offset().top - 80
+ }, 200, function() {
+ $("#story-vid").focus();
+ });
+ } else {
+ e.preventDefault();
+ var $origin = $(this);
+ var content =
+ '<video id="video" poster="'+poster+'" src="'+video+'" controls autoplay type="video/webm">'
+ +'<p class="desc">'+desc+'</p>';
+ videoModal($origin, content);
+ }
+ });
+ }
+
+ function videoModal(origin, content) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

This is really more of a generic modal rather than a video-specific modal, especially because sometimes there is no #video on the page when it is called.

I suggest adding an "onclose" function argument that is run once the done button is clicked or esc key is pressed. Then, you can create a videoModal function:

function videoModal(url, desc) {
    var content = '<video id="video" src="' + url + '"> controls autoplay type="video/webm"><p class="desc">' + desc + '</p>';
    modal(content, function() {
        $('#video')[0].pause();
    });
    $('#video').focus()[0].play();
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ }
+ if ( $("span.btn-play") ) {
+ $("span.btn-play").remove();
+ }
+ $("#video-stage figcaption").fadeOut('fast', function(){ $(this).html(desc).fadeIn('fast'); });
+ $("#story-vid").attr('poster', poster).attr('controls','controls').attr('src', video)[0].play();
+ $('html, body').animate({
+ scrollTop: $("#video-stage").offset().top - 80
+ }, 200, function() {
+ $("#story-vid").focus();
+ });
+ } else {
+ e.preventDefault();
+ var $origin = $(this);
+ var content =
+ '<video id="video" poster="'+poster+'" src="'+video+'" controls autoplay type="video/webm">'
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Also not sure if this needs a poster as it starts playing almost immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ $("#video")[0].pause();
+ $("#fill").remove();
+ $("body").removeClass("noscroll");
+ });
+
+ $("#fill").bind('keyup', function(e) {
+ if (e.keyCode == 27) { // esc
+ $("#fill").remove();
+ $("body").removeClass("noscroll");
+ origin.focus();
+ }
+ });
+ }
+
+ // Remove the full-page modal
+ function closeModal(origin) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

This function is no longer used, remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ $('.btn-prev').attr('disabled','disabled').addClass('disabled');
+ } else {
+ $('.btn-prev').removeAttr('disabled').removeClass('disabled');
+ }
+ if (carousel.last == carousel.size()) {
+ $('.btn-next').attr('disabled','disabled').addClass('disabled');
+ } else {
+ $('.btn-next').removeAttr('disabled').removeClass('disabled');
+ }
+ }
+
+ // Remove the carousel when we don't need it, redo the thumbnails and show the original
+ // This is for smaller viewports that don't get the slider
+ function removeCarousel() {
+ $("#story-slider-clone").parents(".jcarousel-container").remove();
+ if ($("#story-vid")[0].paused == false) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Another unnecessary pause check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ $("#story-slider-clone").jcarousel({
+ scroll: 4,
+ visible: 4,
+ buttonNextHTML: null,
+ buttonPrevHTML: null,
+ itemLastOutCallback: { onAfterAnimation: disableButtons },
+ itemLastInCallback: { onAfterAnimation: disableButtons },
+ initCallback: controlButtons
+ });
+ setStage();
+ setupThumbnails();
+ }
+
+ // Set up video stage
+ function setStage() {
+ var video = $("#story-slider-clone").find("a.contributor:first").attr("href");
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Fix alignment please.

Also, why the find? Can't you just make one big selector?

Also, this function has a few repeated jQuery calls that could be assigned to variables as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ scroll: 4,
+ visible: 4,
+ buttonNextHTML: null,
+ buttonPrevHTML: null,
+ itemLastOutCallback: { onAfterAnimation: disableButtons },
+ itemLastInCallback: { onAfterAnimation: disableButtons },
+ initCallback: controlButtons
+ });
+ setStage();
+ setupThumbnails();
+ }
+
+ // Set up video stage
+ function setStage() {
+ var video = $("#story-slider-clone").find("a.contributor:first").attr("href");
+ var poster = $("#story-slider-clone").find("a.contributor:first").attr("data-poster");
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Use $elem.data('poster') here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose and 1 other commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ buttonNextHTML: null,
+ buttonPrevHTML: null,
+ itemLastOutCallback: { onAfterAnimation: disableButtons },
+ itemLastInCallback: { onAfterAnimation: disableButtons },
+ initCallback: controlButtons
+ });
+ setStage();
+ setupThumbnails();
+ }
+
+ // Set up video stage
+ function setStage() {
+ var video = $("#story-slider-clone").find("a.contributor:first").attr("href");
+ var poster = $("#story-slider-clone").find("a.contributor:first").attr("data-poster");
+ var desc = $("#story-slider-clone").find(".vcard:first .note").html();
+ if ( $("#story-vid").length > 0) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

You shouldn't need to check if these exist before attempting to change the attributes or removing them, doesn't jQuery just do nothing if the selector doesn't match anything?

Same goes for the rest of the function, there's a few more unnecessary checks.

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

Usually yeah but it looks like sometimes the video pause/play stuff throws errors, I think because of the "[0]" part, which shouldn't be necessary but jquery won't play the video without it. I get a "$(...).foo is undefined" error if I try to play or pause a video that doesn't exist.

I think they're redundant in functions that are only called when something is clicked (when the video's existence can be safely assumed), but for stuff that runs on page load the existence checks prevent those errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ var video = $("#story-slider-clone").find("a.contributor:first").attr("href");
+ var poster = $("#story-slider-clone").find("a.contributor:first").attr("data-poster");
+ var desc = $("#story-slider-clone").find(".vcard:first .note").html();
+ if ( $("#story-vid").length > 0) {
+ $("#story-vid").attr('poster', poster).attr('src', video);
+ }
+ if ($("#video-stage figcaption").length > 0) {
+ $("#video-stage figcaption").remove();
+ }
+ $("#video-stage").append('<figcaption>'+desc+'</figcaption>');
+
+ // Add a play button overlay
+ if ($("span.btn-play").length > 0) {
+ $("span.btn-play").remove();
+ }
+ $("#video-stage .player").append('<span class="btn-play"></span>');
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

A bit prettier:

$('<span class="btn-play"></span>').appendTo('#video-stage .player').click(function() {
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose and 1 other commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ } else {
+ if(fixed) {
+ fixed = false;
+ $nav.removeAttr("class");
+ $nav.find("li").removeClass();
+ $("#nav-welcome").addClass("current");
+ $head.css({ "margin-bottom" : "0" });
+ }
+ }
+ }
+ }
+
+ // Check for an adjusted scrollbar every 100ms.
+ setInterval(adjustScrollbar, 100);
+
+ if (!wideMode) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

A comment explaining this block would be lovely.

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

This shows and hides the navigation on small viewports. Comment added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Osmose Osmose and 1 other commented on an outdated diff Nov 13, 2012
media/js/annual2011.js
+ +'</video>';
+ videoModal($origin, content);
+ });
+
+ // Load the YouTube video in a full-page modal
+ $("a.vid-youtube").click( function(e) {
+ e.preventDefault();
+ var $origin = $(this);
+ var content = '<iframe width="640" height="360" src="http://www.youtube-nocookie.com/embed/f_f5wNw-2c0?rel=0" frameborder="0" allowfullscreen></iframe>';
+ videoModal($origin, content);
+ });
+
+ // Set up the contributor stories carousel
+ function setupCarousel() {
+ $sliderPrime.clone().attr("id", "story-slider-clone").insertAfter($("#video-stage"));
+ if ( ($("#story-vid").length > 0) && ($("#story-vid")[0].paused == false) ) {
@Osmose
Mozilla member
Osmose added a line comment Nov 13, 2012

Yet another possibly unnecessary existence check.

@craigcook
Mozilla member
craigcook added a line comment Nov 13, 2012

The pause check was redundant but it looks like it still needs the existence check or I get an error on the FAQ page where the video doesn't exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
craigcook added some commits Nov 13, 2012
@craigcook craigcook More review changes, bug fixes
* Added financial statement PDF (still waiting for 990)
* Fixed bug 810997 (transparency on android)
0235013
@craigcook craigcook Some whitespace cleanup to appease mkelly e89ece3
@craigcook craigcook Optimized some images 526e19a
@sgarrity sgarrity commented on the diff Nov 13, 2012
media/css/foundation/annual2011.less
+
+/* Intro ***/
+.mob-topics {
+ .open-sans-light;
+ background: #000;
+ color: #fff;
+ font-size: 24px;
+ padding: 40px 20px;
+ margin: 0;
+ text-align: center;
+ overflow: hidden;
+ position: relative;
+ z-index: 1;
+ clear: both;
+ li {
+ display: inline-block;
@sgarrity
sgarrity added a line comment Nov 13, 2012

I'm not sure it matters in this case, but we do have a .inline-block(); LESS mixin that outputs the IE fallback hacks too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sgarrity

I've shared a few minor nit-picks with @craigcook in IRC. As well as one can review 1,000 lines of LESS in one sitting, things look good. r+ on the LESS code.

@craigcook craigcook Fixed faq letter-spacing
A more correct solution is to fix this up the chain in sandstone where dt elements are 32px with -1px letter-spacing. But I don't want to change that now because we'd have to fix any other pages already using that styling.

Note to self: fix dts in sandstone.
064ebf0
@Osmose
Mozilla member

r+ on the JS code, anything left is nits that shouldn't block. Awesome work! 🍰

@rik rik merged commit 7beb38d into mozilla:master Nov 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment