Closed Bug 1090727 Opened 6 years ago Closed 6 years ago

support jquery in the global/header template, and update the header and footer to use jquery

Categories

(Bugzilla :: Bugzilla-General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: glob, Assigned: dkl)

References

Details

Attachments

(1 file, 2 obsolete files)

the global/header template needs updating to support loading jquery.

rough notes:
- load both jquery and yui2 on every page by default
- new header param "jqlibs" (or "jslibs"?)
  - initially support for jquery-ui only (loads smoothness css + js)
- new header param "noyui"
  - so pages can opt out of loading yui2
  - at some stage we need to invert this, so pages need to opt into yui2
- convert header, footer, and all dependent templates/js to jquery
  - required in order for pages to opt-out
  - yui hacks, such as onpagehide, can be removed if "noyui" is set
  - to simplify testing and reviewing avoid refactoring of code, a straight port is most useful
Blocks: 1068655
Severity: normal → enhancement
OS: Mac OS X → All
Hardware: x86 → All
Attached patch 1090727_1.patch (obsolete) — Splinter Review
Working proof of concept of incorporating latest jQuery along side YUI in the Bugzilla code. I have converted the JS that is present in header/footer to JQuery such as js/global.js, etc. I am sure we can improve on what I have here so far but wanted to get it up for feedback.

dkl
Attachment #8520881 - Flags: feedback?(glob)
Comment on attachment 8520881 [details] [diff] [review]
1090727_1.patch

Review of attachment 8520881 [details] [diff] [review]:
-----------------------------------------------------------------

nice, this looks like an excellent start.
note you have the jquery-cookie.js plugin outside of the plugins directory; it would be cleaner to minify it and move it into js/jquery/plugins

one gotcha with migration is YUI's tools will ignore requests with invalid/missing elements, while jquery will throw an error:
> YAHOO.util.Dom.removeClass('monkey', 'banana')
> (returns false)
> $('#monkey').removeClass('banana')
> TypeError: $(...) is null
from just reading the header code here i think it's ok, but this would require testing.
Attachment #8520881 - Flags: feedback?(glob) → feedback+
Attached patch 1090727_2.patch (obsolete) — Splinter Review
new patch that has minified cookie-min.js and also uses 'use_yui' param (default on) so that some pages can opt out of loading YUI.

Looking through the code, I feel the next phase after this bug can simply be replacing all use of YAHOO.util.Dom.*, YAHOO.util.Event.*, YAHOO.util.JSON.*, etc. with the jQuery counterparts. Some js/*.js files are simply using those util methods and would be easy ports.

The next stage will be more complex such as re-implementing the autocomplete, dupe search and other functions. But I suggest we tackle all low hanging fruit and work out way up.

> one gotcha with migration is YUI's tools will ignore requests with
> invalid/missing elements, while jquery will throw an error:
> > YAHOO.util.Dom.removeClass('monkey', 'banana')
> > (returns false)
> > $('#monkey').removeClass('banana')
> > TypeError: $(...) is null
> from just reading the header code here i think it's ok, but this would
> require testing.

I did not notice this in my testing. I edited some lines in js/global.js to look for non-existent IDs and did not see an error in the logs. Of course the div was not hidden as predicted but no error was generated. Will try some other things to reproduce.

dkl
dkl
Attachment #8520881 - Attachment is obsolete: true
Attachment #8521614 - Flags: review?(glob)
Comment on attachment 8521614 [details] [diff] [review]
1090727_2.patch

Review of attachment 8521614 [details] [diff] [review]:
-----------------------------------------------------------------

this looks great.  just a few things to sort out..

::: template/en/default/global/header.html.tmpl
@@ +32,4 @@
>    onload = ""
>    style_urls = []
>    yui = []
> +  use_yui = 1

you can't DEFAULT a boolean to true, because they are impossible to set to false when PROCESSing the header.

@@ +66,5 @@
>    # every other JS URL.
>    #%]
>  [% SET starting_js_urls = [
> +    "js/jquery/jquery-min.js",
> +    "js/jquery/plugin/cookie/cookie-min.js",

the cookie plugin should be pushed onto the 'jquery' array, not starting_js_urls.

this will also fix the typo in the path (s/plugin/plugins/), which generates a javascript error when switching languages.

@@ +85,5 @@
> +  [% jq_css_urls.push(jq_css_url) %]
> +[% END %]
> +[% FOREACH jq_css_url = jq_css_urls %]
> +  [% style_urls.push(jq_css_url) %]
> +[% END %]

instead of two FOREACHes you can use .import:

[% undef = style_urls.import(jq_css_urls, jquery_css) %]
Attachment #8521614 - Flags: review?(glob) → review-
Attached patch 1090727_3.patchSplinter Review
Attachment #8521614 - Attachment is obsolete: true
Attachment #8522506 - Flags: review?(glob)
Comment on attachment 8522506 [details] [diff] [review]
1090727_3.patch

Review of attachment 8522506 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

during testing i discovered we're breaking "data:" urls which jquery-ui uses; filed as bug 1100368.

::: template/en/default/global/header.html.tmpl
@@ +68,5 @@
>    # every other JS URL.
>    #%]
>  [% SET starting_js_urls = [
> +    "js/jquery/jquery-min.js",
> +    "js/jquery/ui/jquery-ui-min.js"

nit: indentation here is weird; should be 2 spaces not 4.

@@ +79,5 @@
> +
> +[% SET jq_css_urls = [
> +    "js/jquery/ui/jquery-ui-min.css",
> +    "js/jquery/ui/jquery-ui-structure-min.css",
> +    "js/jquery/ui/jquery-ui-theme-min.css",

same indentation issue here

@@ +84,2 @@
>  ] %]
> +[% undef = style_urls.import(jquery_css, jq_css_urls) %] 

remove trailing whitespace
Attachment #8522506 - Flags: review?(glob) → review+
Flags: approval+
Target Milestone: --- → Bugzilla 6.0
Blocks: 1100382
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   11c2093..a50e8fd  master -> master
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1102003
Comment on attachment 8522506 [details] [diff] [review]
1090727_3.patch

>+++ b/template/en/default/global/header.html.tmpl

>+[% undef = style_urls.import(jquery_css, jq_css_urls) %] 

Err... what does [% undef = ... %] mean???
You need to log in before you can comment on or make changes to this bug.