302702 - factor out tabs code

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: 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 2

[Max Kanat-Alexander]

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.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: 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 4

[Max Kanat-Alexander]

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

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: fixes style issues

Oops, didn't notice that.  Here's the fix.

Comment 6

[Max Kanat-Alexander]

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

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: 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 8

[Max Kanat-Alexander]

Comment on attachment 191007 [details] [diff] [review]
patch v4: doesn't compare objects

Yep, this works on landfill. :-)

Comment 9

[Myk Melez [:myk] [@mykmelez]]

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

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: build bustage fix

I subsequently checked in this fix for the build bustage caused by my checkin.