migrate js/TUI.js and js/util.js to jquery.

hi glob I am migrating TUI.js to jquery. I am stuck on cookie implementation part. YUI is using subcookies but carhart jquery plugin has no concept of subcookies. how should i implement subcookies in jquery? there is a pluging called ezcookie for subcookie implementation. https://code.google.com/p/ezcookie/ . Should i use that ?

as TUI is the only place where subcookies are used, i'm not sure it makes sense to change cookie libraries bugzilla-wide for this, especially with ezcookie appearing to be a dead project. yui's subcookie implementation is just a query string, so you can use jquery's param method to generate the full cookie string, and something like the following to parse it: jQuery.deparam = function(params) { var o = {}; if (!params) return o; var a = params.split('&'); for (var i = 0; i < a.length; i++) { var pair = a[i].split('='); o[decodeURIComponent(pair[0])] = decodeURIComponent(pair[1]); } return o; }

TUI.js file has been migrated to jquery. json and carhart jquery plugin have been used to implement subccokies.

YUI code in util.js has been converted to jquery code.

Please attach patches, i.e. generate a diff of the old vs new code.

TUI.js file has been migrated to jquery. json and carhart jquery plugin have been used to implement subccokies.

YUI code in util.js has been converted to jquery code.

Comment on attachment 8564513 [details] [diff] [review] patch.diff Do not forget to request review, else your patch won't appear in reviewers' radar.

Comment on attachment 8564514 [details] [diff] [review] patch1.diff > function bz_toggleClass(anElement, aClass) { >+ if (element.hasClass(aclass)) { >+ element.removeClass(aclass); You pass 'anElement' to the function, but then use 'element'?

YUI code in util.js has been converted to jquery code. Sorry for the silly mistake.

Comment on attachment 8564513 [details] [diff] [review] patch.diff oops; you've split the patch into two files. resurrecting this attachment.

Comment on attachment 8564828 [details] [diff] [review] patch1_2.diff Review of attachment 8564828 [details] [diff] [review]: ----------------------------------------------------------------- thanks for this patch, it looks like a nice start. there are a lot of formatting changes here which are not related to this change; please only change the required parts of the file. also, for future revisions, please attach a single patch which contains updates to both files. ::: js/util.js @@ +315,1 @@ > } this assumes the element passed in will be a jquery object, which isn't the case. it's called with the element id, so you need to look that up here.

YUI.js and TUI.js are now contained inside a single patch file.

(In reply to Byron Jones ‹:glob› from comment #12) > Comment on attachment 8564828 [details] [diff] [review] > patch1_2.diff > > Review of attachment 8564828 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/util.js > @@ +315,1 @@ > > } > > this assumes the element passed in will be a jquery object, which isn't the > case. it's called with the element id, so you need to look that up here. I think I am passing a jquery element in bz_toggleClass function as a parameter. I am calling bz_toggleClass in TUI_toggle_class function of TUI.js and passing a jquery element only. $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS));

(In reply to Kapil Bakshi[geekspark] from comment #14) > I think I am passing a jquery element in bz_toggleClass function as a > parameter. I am calling bz_toggleClass in TUI_toggle_class function of > TUI.js and passing a jquery element only. > $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS)); that's correct, however bz_toggleClass is used in other places in bugzilla, as well as potentially in extensions. you cannot change the type of parameter these functions expect.

(In reply to Byron Jones ‹:glob› from comment #15) > (In reply to Kapil Bakshi[geekspark] from comment #14) > > I think I am passing a jquery element in bz_toggleClass function as a > > parameter. I am calling bz_toggleClass in TUI_toggle_class function of > > TUI.js and passing a jquery element only. > > $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS)); > > that's correct, however bz_toggleClass is used in other places in bugzilla, > as well as potentially in extensions. you cannot change the type of > parameter these functions expect. so I shouldn't pass a jquery element here. and call this function as it was being called earlier ?

Comment on attachment 8565316 [details] [diff] [review] patch2.diff Review of attachment 8565316 [details] [diff] [review]: ----------------------------------------------------------------- > so I shouldn't pass a jquery element here. and call this function as it was being called earlier ? correct. this work should be limited to just replacing yui and jquery. changing function signatures will break code, and other refactoring makes reviewing and testing difficult. there's a few syntax errors here which prevent your code from running. please ensure you've tested your code prior to submitting a patch. ::: js/TUI.js @@ +22,4 @@ > > var TUI_alternates = new Array(); > > +var json_subcookies = {}; these cookies aren't json, it's a normal js object. let's call this variable BUGZILLA.subcookies @@ +32,4 @@ > * @param className The name of the CSS class to hide. > */ > function TUI_toggle_class(className) { > + $("."+className).each( bz_toggleClass($(this), TUI_HIDDEN_CLASS)); add spaces around the + "." + className this issue appears multiple times in this patch. @@ +50,5 @@ > + if(json_subcookies !=null) > + { > + if(json_subcookies[className] != 0){ > + TUI_toggle_class(className); > + } the formatting of this block needs some minor attention. - please remove the trailing whitespace - add a space between if and ( - ensure there's a space after = - ensure there's a space between ) and { - bring the { up to the same line as the if @@ +89,5 @@ > + var cookie = $.deparam($.cookie(TUI_COOKIE_NAME, { raw: true })); > + for (cookie_item in cookie) { > + if (cookie[cookie_item] == 0) { > + var elements = $("."+cookie_item); > + elements.each(function{ this has a syntax errox. @@ +106,5 @@ > + var pair = a[i].split('='); > + o[decodeURIComponent(pair[0])] = decodeURIComponent(pair[1]); > + } > + return o; > +} missing ; after the } ::: js/util.js @@ +313,3 @@ > * @param aClass The name of the CSS class to toggle. > */ > +function bz_toggleClass(element, aClass) { as per my previous review, you cannot change a function's signature. @@ +321,1 @@ > } "aclass" is not defined. it's "aClass"

All the issues mentioned in comment 17 have been resolved .

All the issues mentioned in comment 17 have been resolved . bz_toggleClass function in util.js now has a javascript DOM element whose class is to be toogled as one of its parameter instead of a jquery one .

All the issues mentioned in comment 17 have been resolved . bz_toggleClass function in util.js now has a javascript DOM element (whose class is to be toogled) as one of its parameter instead of a jquery one .

Comment on attachment 8567101 [details] [diff] [review] patch3_3.diff Review of attachment 8567101 [details] [diff] [review]: ----------------------------------------------------------------- oops; this patch appears to have been generated using the previous patch as the parent. please provide a patch using trunk as a base. ::: js/util.js @@ +311,5 @@ > * @param anElement The element to toggle the class on > * @param aClass The name of the CSS class to toggle. > */ > +function bz_toggleClass(anElement, aClass) { > + anElement.classList.toggle(aClass); sorry, this still won't work. look at other places that call this code: extensions/Voting/template/en/default/hook/admin/products/edit-common-rows.html.tmpl 43: function() { bz_toggleClass('votes_to_confirm_container', template/en/default/admin/params/common.html.tmpl 87: bz_toggleClass("input_[% param.name FILTER html %]", "bz_default_hidden"); 88: bz_toggleClass("table_[% param.name FILTER html %]", "bz_default_hidden"); template/en/default/attachment/list.html.tmpl 23: bz_toggleClass(rows[i], 'bz_default_hidden'); all of these need to continue to work, which means bz_toggleClass needs to continue to support accepting either an id string *or* a vanilla DOM element as its parameter. note: just changing the places quoted above isn't sufficient, as this may break third party extensions unnecessarily. you want something like (untested): var el = typeof anElement === 'object' ? $(anElement) : $('#' + anElement); el.toggle(aClass); also note DOM's toggle() method isn't supported on older IE versions (requires v10), so you can't use it. use jquery's toggle instead.

Sorry was busy with exams. bz_toggleClass method in util.js is now handling parameters being passed properly.

Glob Could you please let me know whether my submitted patch is correct or not ?

(In reply to Kapil Bakshi[geekspark] from comment #23) > Glob Could you please let me know whether my submitted patch is correct or > not ? sorry; i have some other higher priority issues that i'm working on right now. a cursory read of the patch looks good, but you'll have to wait a little longer for a full review i'm sorry.

(In reply to Byron Jones ‹:glob› from comment #24) > sorry; i have some other higher priority issues that i'm working on right > now. > a cursory read of the patch looks good, but you'll have to wait a little > longer for a full review i'm sorry. Would you be able to review the patch now ?

Comment on attachment 8571103 [details] [diff] [review] patch4.diff Review of attachment 8571103 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/TUI.js @@ +18,4 @@ > */ > > var TUI_HIDDEN_CLASS = 'bz_tui_hidden'; > +var TUI_COOKIE_NAME = 'TUI'; this change is not required - the equal signs look nice in when in alignment. @@ +22,5 @@ > > var TUI_alternates = new Array(); > > + > + this change is not required - only need a single blank line here, not three. @@ +36,3 @@ > for (var i = 0; i < elements.length; i++) { > bz_toggleClass(elements[i], TUI_HIDDEN_CLASS); > } unfortunately getElementsByClassName isn't supported by IE8. try something like (completely untested; you'll need to check this works): $('.' + className).each(function() { bz_toggleClass(self, TUI_HIDDEN_CLASS); } @@ +62,1 @@ > if (!link) return; this will always be true, because $(..) returns an array. test link.length (testing should have picked this up) @@ +77,5 @@ > } > > function _TUI_store(aClass, state) { > + > + BUGZILLA.subcookies[aclass] = state; ReferenceError: aclass is not defined and then when that's fixed: TypeError: BUGZILLA.subcookies is undefined please test your patches. @@ +87,5 @@ > } > > function _TUI_restore() { > + var cookie = $.deparam($.cookie(TUI_COOKIE_NAME, { raw: true })); > + for (cookie_item in cookie) { this creates a global 'for' variable. it should be for (var cookie_item in ..)

geekspark: are you still working on this? :-) Gerv

geekspark: I would understand if you were fed up with this, but please do let us know: are you able to carry on working on these patches? We'd like to get migrated to YUI before we release the next version of Bugzilla, and it seems like this bug is necessary for that. Thanks :-) Gerv

Since there has been no activity for the last two months, I'll assume the assignee has no time for this. I want us (the project) to learn one thing from this (and similar bugs, with half-completed patches that the patch author didn't test): Sure, the patch author *should* test, but we have a responsibility as a project to make it easy test patches if we expect patches from volunteers. So, thanks :geekspark for the attempt! if you're in our neck of the woods again and would like help getting a working bugzilla dev instance set up -- if you have any issues with that -- let us know!

Comment on attachment 8790126 [details] [bugzilla] dylanwh:bug-1128408 > bugzilla:master This changes to jquery and moves the storage location of TUI visibility data from a cookie to localStorage. This is a feature -- getting rid of extraneous cookies is a good thing -- at the small inconvenience of forgetting visibility settings.

gerv: as you have maybe never done a pull request before, you can copy the pull request url and add '.patch' to the end to download a fairly traditional patch file to apply to a recent-ish bugzilla checkout. Splinter was working for pull requests but something is broken and dkl hasn't figured it out yet.

Comment on attachment 8790126 [details] [bugzilla] dylanwh:bug-1128408 > bugzilla:master I realize this patch is entirely js, so based on past discussions it's acceptable for kohei to take a stab at reviewing if he has time and interest. So setting r?

(In reply to Dylan Hardison [:dylan] from comment #32) > gerv: as you have maybe never done a pull request before, you can copy the > pull request url and add '.patch' to the end to download a fairly > traditional patch file to apply to a recent-ish bugzilla checkout. Splinter > was working for pull requests but something is broken and dkl hasn't figured > it out yet. Thanks for the tip; that's very useful for my patch-downloading-and-applying script. r=gerv on Github, although it seems that doesn't propagate here? Gerv

Comment on attachment 8790126 [details] [bugzilla] dylanwh:bug-1128408 > bugzilla:master r=gerv if comments on Github are addressed. Gerv

Comment on attachment 8790126 [details] [bugzilla] dylanwh:bug-1128408 > bugzilla:master This actually won't work because the json structure gets cached per tab. We need to deserialize on every update to keep it in sync.

We should remove jQuery as well, just like GitHub.