The tabs code could be useful elsewhere in Bugzilla besides just prefs and searches. It should be factored out so both those places use the same code and other places can also use it.
Created attachment 190995 [details] [diff] [review] patch v1: factors out tab code Refactors code into globals/tabs.html.tmpl, modifies prefs.html.tmpl and search/tabs.html.tmpl to use the refactored version, and refactors CSS into skins/standard/global.css.
Comment on attachment 190995 [details] [diff] [review] patch v1: factors out tab code It looks like the CSS part of the patch didn't come through. (That is, you deleted the selected_tab parts from some CSS, but didn't add it back anywhere?) >+[% FOREACH tab IN tabs %] >+ [% IF tab.name == current_tab_name %] >+ [% current_tab=tab %] >+ [% LAST %] >+ [% END %] >+[% END %] Nit: Could we refactor that slightly to not need that code? It seems like something better done in the tab template. Or alternately, the tab object could be a hash where the keys were the names...? Otherwise, this code *looks* great. :-) I need to test it, still, though.
Created attachment 190999 [details] [diff] [review] patch v2: with the CSS changes >Nit: Could we refactor that slightly to not need that code? It seems like >something better done in the tab template. Unfortunately it's the prefs template that actually needs current_tab to be the tab hash for the current tab. The tab template would be happy receiving just the name of the current tab and finding out which hash that name refers to while iterating through the list of tab hashes. But the prefs template uses the hash later, so it needs to have a reference to it, and since the hashes collection is a list, we have to iterate through it to find the one we want. >Or alternately, the tab object could be a hash where the keys were the >names...? We could do that, but then we'd need a "sortkey" to make sure we display the tabs in the right order, which is itself kludgy. Here's the patch with the CSS changes included. I also fixed a couple references to tab.description in prefs.html.tmpl, which should be tab.label now.
Comment on attachment 190999 [details] [diff] [review] patch v2: with the CSS changes Slight style problem: the gray background now extends beyond the tabs, which looks a bit strange: http://landfill.bugzilla.org/mkanat/query.cgi
Created attachment 191002 [details] [diff] [review] patch v3: fixes style issues Oops, didn't notice that. Here's the fix.
Comment on attachment 191002 [details] [diff] [review] patch v3: fixes style issues Hrm. With this patch, tab highlighting seems to no longer work on the Prefs page: http://landfill.bugzilla.org/mkanat/userprefs.cgi?tab=settings
Created attachment 191007 [details] [diff] [review] patch v4: doesn't compare objects On the theory that object comparison doesn't work on Template 2.08 (the version on landfill) but does on 2.14 (the version on my local machine), here's a version that doesn't compare objects.
Comment on attachment 191007 [details] [diff] [review] patch v4: doesn't compare objects Yep, this works on landfill. :-)
Thanks mkanat! Checking in template/en/default/account/prefs/prefs.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/prefs.html.tmpl,v <-- prefs.html.tmpl new revision: 1.19; previous revision: 1.18 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/tabs.html.tmpl,v done Checking in template/en/default/global/tabs.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/tabs.html.tmpl,v <-- tabs.html.tmpl initial revision: 1.1 done Checking in template/en/default/search/search-advanced.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-advanced.html.tmpl,v <-- search-advanced.html.tmpl new revision: 1.24; previous revision: 1.23 done Checking in template/en/default/search/search-specific.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-specific.html.tmpl,v <-- search-specific.html.tmpl new revision: 1.13; previous revision: 1.12 done Checking in template/en/default/search/tabs.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/tabs.html.tmpl,v <-- tabs.html.tmpl new revision: 1.5; previous revision: 1.4 done Checking in skins/standard/global.css; /cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v <-- global.css new revision: 1.13; previous revision: 1.12 done
Created attachment 191011 [details] [diff] [review] build bustage fix I subsequently checked in this fix for the build bustage caused by my checkin.