factor out tabs code

RESOLVED FIXED in Bugzilla 2.22

Status

()

Bugzilla
User Interface
--
enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: myk, Assigned: myk)

Tracking

2.21
Bugzilla 2.22
Bug Flags:
approval +

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

13 years ago
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.

Updated

13 years ago
Severity: normal → enhancement
OS: Linux → All
Hardware: PC → All
Version: 2.18.1 → 2.21
(Assignee)

Comment 1

13 years ago
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.
(Assignee)

Updated

13 years ago
Attachment #190995 - Flags: review?(mkanat)

Comment 2

13 years ago
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.
Attachment #190995 - Flags: review?(mkanat) → review-
(Assignee)

Comment 3

13 years ago
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.
Attachment #190995 - Attachment is obsolete: true
Attachment #190999 - Flags: review?(mkanat)

Comment 4

13 years ago
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
Attachment #190999 - Flags: review?(mkanat) → review-
(Assignee)

Comment 5

13 years ago
Created attachment 191002 [details] [diff] [review]
patch v3: fixes style issues

Oops, didn't notice that.  Here's the fix.
Attachment #190999 - Attachment is obsolete: true
Attachment #191002 - Flags: review?(mkanat)

Comment 6

13 years ago
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
Attachment #191002 - Flags: review?(mkanat) → review-
(Assignee)

Comment 7

13 years ago
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.
Attachment #191002 - Attachment is obsolete: true
Attachment #191007 - Flags: review?(mkanat)

Comment 8

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

Yep, this works on landfill. :-)
Attachment #191007 - Flags: review?(mkanat) → review+
(Assignee)

Comment 9

13 years ago
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
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Flags: approval+
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 2.22
(Assignee)

Comment 10

13 years ago
Created attachment 191011 [details] [diff] [review]
build bustage fix

I subsequently checked in this fix for the build bustage caused by my checkin.
You need to log in before you can comment on or make changes to this bug.