Bug 918260 - Update jQuery to 1.11.0 in mozorg #1651

Merged
merged 1 commit into from Feb 25, 2014

Conversation

Projects
None yet
2 participants
@cinemascop89
Contributor

cinemascop89 commented Feb 1, 2014

This updates jQuery to 1.11.0 for the mozorg app, all templates where checked using the jQuery migrate plugin.

media/js/base/site.js
@@ -3,6 +3,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
(function () {
'use strict';
+

This comment has been minimized.

@alexgibson

alexgibson Feb 4, 2014

Member

Remove new line

@alexgibson

alexgibson Feb 4, 2014

Member

Remove new line

media/js/base/site.js
@@ -81,7 +82,7 @@
// to avoid lots of flickering
var h = document.documentElement;
window.site = {
- platform : getPlatform()
+ platform : getPlatform(),

This comment has been minimized.

@alexgibson

alexgibson Feb 4, 2014

Member

Please remove the trailing comma

@alexgibson

alexgibson Feb 4, 2014

Member

Please remove the trailing comma

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Feb 4, 2014

Member

Searching bedrock for usage of the mozorg-resp JS bundle, it seems both firefox/partners and firefox/os are using it. Both these page templates should probably be changed to using firefox-resp, as they do not reside in the mozorg app. The only difference in the bundles is that firefox-resp also includes mozilla-input-placeholder.js, which should be fine.

Note: looks like you may need to remove mozilla-input-placeholder.js from the firefox_os bundle once you do this, to prevent the file being included twice.

Member

alexgibson commented Feb 4, 2014

Searching bedrock for usage of the mozorg-resp JS bundle, it seems both firefox/partners and firefox/os are using it. Both these page templates should probably be changed to using firefox-resp, as they do not reside in the mozorg app. The only difference in the bundles is that firefox-resp also includes mozilla-input-placeholder.js, which should be fine.

Note: looks like you may need to remove mozilla-input-placeholder.js from the firefox_os bundle once you do this, to prevent the file being included twice.

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Feb 4, 2014

Member

On pages that include jQuery 1.11, if you resize the browser and open the mobile navigation using the hamburger menu, you get the following error:

TypeError: $(...).get(...) is undefined

Looks to be this line that is the issue: https://github.com/mozilla/bedrock/blob/master/media/js/base/nav-main-resp.js#L409

Think it might be an issue with the [tabindex=0] selector.

Member

alexgibson commented Feb 4, 2014

On pages that include jQuery 1.11, if you resize the browser and open the mobile navigation using the hamburger menu, you get the following error:

TypeError: $(...).get(...) is undefined

Looks to be this line that is the issue: https://github.com/mozilla/bedrock/blob/master/media/js/base/nav-main-resp.js#L409

Think it might be an issue with the [tabindex=0] selector.

@cinemascop89

This comment has been minimized.

Show comment
Hide comment
@cinemascop89

cinemascop89 Feb 5, 2014

Contributor

I can't seem to find what's wrong with that selector, can't find anything in the jQuery changelogs, any suggestions?

Contributor

cinemascop89 commented Feb 5, 2014

I can't seem to find what's wrong with that selector, can't find anything in the jQuery changelogs, any suggestions?

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Feb 5, 2014

Member

I can't seem to find what's wrong with that selector, can't find anything in the jQuery changelogs, any suggestions?

It's an odd one I can't find any documented info on this change also, although I believe here we should be able to use $('#nav-main-menu li > a:first').focus(); as a suitable replacement.

I'll see if I can find out why the selector no longer works in this instance, but no reason to hold up fixing this one thing.

Member

alexgibson commented Feb 5, 2014

I can't seem to find what's wrong with that selector, can't find anything in the jQuery changelogs, any suggestions?

It's an odd one I can't find any documented info on this change also, although I believe here we should be able to use $('#nav-main-menu li > a:first').focus(); as a suitable replacement.

I'll see if I can find out why the selector no longer works in this instance, but no reason to hold up fixing this one thing.

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Feb 5, 2014

Member

After some debugging, it seems '#nav-main-menu [tabindex=0]' will only work in jQuery 1.11 if the tabindex attribute is present on the link in the actual DOM.

Member

alexgibson commented Feb 5, 2014

After some debugging, it seems '#nav-main-menu [tabindex=0]' will only work in jQuery 1.11 if the tabindex attribute is present on the link in the actual DOM.

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Feb 13, 2014

Member

Hi, @cinemascop89 - just checking in on this PR. Have you managed to make some progress? If there's anything I can help with please just ask. Thanks!

Member

alexgibson commented Feb 13, 2014

Hi, @cinemascop89 - just checking in on this PR. Have you managed to make some progress? If there's anything I can help with please just ask. Thanks!

@cinemascop89

This comment has been minimized.

Show comment
Hide comment
@cinemascop89

cinemascop89 Feb 14, 2014

Contributor

hi, sorry for the silence, been out of home and without internet access, ill be able to finish this in a few days

Date: Thu, 13 Feb 2014 10:11:35 -0800
From: notifications@github.com
To: bedrock@noreply.github.com
CC: eduardo@importth.is
Subject: Re: [bedrock] Bug 918260 - Update jQuery to 1.11.0 in mozorg (#1651)

Hi, @cinemascop89 - just checking in on this PR? Have you managed to make some progress, if there's anything I can help with please just ask. Thanks!


Reply to this email directly or view it on GitHub.

Contributor

cinemascop89 commented Feb 14, 2014

hi, sorry for the silence, been out of home and without internet access, ill be able to finish this in a few days

Date: Thu, 13 Feb 2014 10:11:35 -0800
From: notifications@github.com
To: bedrock@noreply.github.com
CC: eduardo@importth.is
Subject: Re: [bedrock] Bug 918260 - Update jQuery to 1.11.0 in mozorg (#1651)

Hi, @cinemascop89 - just checking in on this PR? Have you managed to make some progress, if there's anything I can help with please just ask. Thanks!


Reply to this email directly or view it on GitHub.

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Feb 24, 2014

Member

Thanks for the update, @cinemascop89!

These changes look great - before we can merge your PR, can you please squash your 5 commits into 1? We have instructions on how to do that here: http://bedrock.readthedocs.org/en/latest/contribute.html#squashing-your-commits

If you need some help with this, please just ask!

Member

alexgibson commented Feb 24, 2014

Thanks for the update, @cinemascop89!

These changes look great - before we can merge your PR, can you please squash your 5 commits into 1? We have instructions on how to do that here: http://bedrock.readthedocs.org/en/latest/contribute.html#squashing-your-commits

If you need some help with this, please just ask!

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Feb 25, 2014

Member

r+ thanks @cinemascop89 🚀

Member

alexgibson commented Feb 25, 2014

r+ thanks @cinemascop89 🚀

alexgibson added a commit that referenced this pull request Feb 25, 2014

Merge pull request #1651 from cinemascop89/918260-jquery-update
Bug 918260 - Update jQuery to 1.11.0 in mozorg

@alexgibson alexgibson merged commit 66c50e9 into mozilla:master Feb 25, 2014

1 check passed

default Jenkins build 'bedrock_github' #3296 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment