Closed Bug 244265 Opened 16 years ago Closed 16 years ago

Abstract out the typical admin page table to a separate template

Categories

(Bugzilla :: Administration, task)

2.17.7
task
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: jouni, Assigned: jouni)

References

Details

Attachments

(1 file, 2 obsolete files)

As discussed earlier in editkeywords.cgi templatization (bug 190223 comment 6
and onwards), most of the admin pages have a table structure displaying a list
of something (products, keywords, milestones, whatnot). The table code gets
repeated pretty often, so we could use an abstraction before doing too many
admin pages.

I'll attach a first stab soon. The patch does the following:

- add admin/table.html.tmpl, a generic structure for printing out tables on the
  admin pages. This could also be in global/; do you think it would be useful
  outside the admin domain? Theoretically there's nothing admin-specific here.

- change admin/keywords/list.html.tmpl to use this new table system
  Also fixes a single comment mistake in mentioned template (s/keyword_id/id/).

- Gets rid of the max_table_size. I'm ready to add it if we really need it, but
  it's not as necessary as it was before (more connection speed, better
  browsers), and I don't think I've ever seen an admin page with the splitting
  actually done. It's looks just like unnecessary clutter to me.
Attached patch v1 (obsolete) — Splinter Review
Oh, I forgot to mention: this simplifies the output a bit: a missing keyword
description is no longer output as a red "missing". I didn't feel it to be
necessary, and didn't want to stuff the generic table structure with output
formatting just yet.
Comment on attachment 149027 [details] [diff] [review]
v1

Joel, you promised to review anything templatization related ;-)
Attachment #149027 - Flags: review?(bugreport)
Attachment #149027 - Flags: review?(gerv)
Comment on attachment 149027 [details] [diff] [review]
v1

r=joel
Attachment #149027 - Flags: review?(bugreport) → review+
Comment on attachment 149027 [details] [diff] [review]
v1

Can we use this opportunity to move the "Add" link to somewhere more visible?
It's really hard to discover down in the corner of the table.

I say we remove it from the table entirely, abolish the "footer", and instead
just have a convention about how the Add link should be placed in the base
page. Somewhere near the top, where it's easy to find.

- "literalcontent" seems unnecessarily verbose - just "content" would do IMO.
People can see it's an override.

- could "data" be an array of arrays? Which is easier and more efficient to get
out of the database, I wonder?

- Do table_header and table_footer need to be named blocks rather than just
inline? They are only used once.

- You don't seem to be using the "align" attribute you've put on the "Bugs"
column.

Gerv
Attachment #149027 - Flags: review?(gerv) → review-
(In reply to comment #4)
> (From update of attachment 149027 [details] [diff] [review])
> Can we use this opportunity to move the "Add" link to somewhere more visible?
> It's really hard to discover down in the corner of the table.

Actually, I disagree. It's not the best of all places perhaps, but I couldn't
come up with a considerably better position for it with quick fiddling. The add
link being below the list perhaps hides it a bit, but I've never heard anyone
_not_ finding it after all. Usually making people see some examples gives them a
reason to think.

Anyway, I'm not opposed to moving the link if someone comes up with a good
proposal. However, I don't think we should hold this technical patch for that. 

> I say we remove it from the table entirely, abolish the "footer", and instead
> just have a convention about how the Add link should be placed in the base
> page. Somewhere near the top, where it's easy to find.

The footer could be useful for several other things, too, such as showing table
counts et al. Otoh, we don't have such an application ATM, and if you think
keeping the link in the "footer" sucks, we can get rid of it. In the patch
coming soon, I've positioned the link outside the table, but left it down below.
This is the same approach as in editflagtypes.cgi.

> - "literalcontent" seems unnecessarily verbose - just "content" would do IMO.
> People can see it's an override.

Done.

> - could "data" be an array of arrays? Which is easier and more efficient to 
> get out of the database, I wonder?

Array of arrays is probably faster, but there are several aspects why it sucks:

1) It creates a relation between visible field order in templates and field
order in SQL SELECT clause - or alternatively creates a column number mapping,
which is a really confusing thing to maintain. 

2) Not all data is pulled from SQL; constructing hashes manually is way better
than constructing lists with magically-order elements.

3) TT's dot operator syntax allows you to substitute objects instead of hashes,
so you could pass it Keyword.pm instances or whatever. 

4) With arrays, the %x% variable interpolation syntax in contentlink would
become _very_ unreadable indeed.

I've left this as it is.

> - Do table_header and table_footer need to be named blocks rather than just
> inline? They are only used once.

Not necessarily. I left them out as named blocks out of old convention. Changed
in the next patch, although I'm not sure whether the named blocks help more than
confuse (sometimes separating a once-called method/sub is also worth it etc.).
But let's go for simplicity this time.

> - You don't seem to be using the "align" attribute you've put on the "Bugs"
> column.

Good catch. Fixed.
Status: NEW → ASSIGNED
Attached patch v2 (obsolete) — Splinter Review
Attachment #149027 - Attachment is obsolete: true
Attachment #149183 - Flags: review?(gerv)
Comment on attachment 149183 [details] [diff] [review]
v2

Looks great to me! Nice job!

uri --> URI because it's an abbreviation.
Attachment #149183 - Flags: review-
Attached patch v3Splinter Review
URI fixed in interface comments.
Attachment #149183 - Attachment is obsolete: true
Attachment #152409 - Flags: review?(vladd)
Attachment #149183 - Flags: review?(gerv)
Should there be something in there which says 'none', for the case when there is
no data?
Comment on attachment 152409 [details] [diff] [review]
v3

r=vladd

I don't object to keeping things simple the way they are now. A header without
any data looks acceptable.

Although I'd prefer to see a message like "No data found." or something like
that when the table contains no rows, like glob suggested.
Attachment #152409 - Flags: review?(vladd) → review+
I mean GavinS :)
This is what I have in my local copy for the 'no data found' case, in case it can be squeezed into this 
patch. If not, I'll include it in my upcoming patch for editcomponnents templatisation...

[% IF data.size == 0 %]
  <tr><td colspan="[% columns.size %]" align="center"><i>&lt;none&gt;</i></td></tr>
[% END %]
(In reply to comment #12)
> This is what I have in my local copy for the 'no data found' case, in 
> case it can be squeezed into this patch. If not, I'll include it in my 
> upcoming patch for editcomponnents templatisation...

Merits of this were discussed on IRC. I agree the current solution of just
showing the table headers isn't perhaps the best one, but I don't think adding
anything semantically empty "none" style stuff is going to help much.

When it's necessary to show a clear message about lack of entries, we might also
IF-branch in the calling template - I'm not certain on the merits this generic
table structure can offer wrt that need. Gavin, when you're implementing
something like this, please CC me so we can discuss it more. 

For now, I'm requesting approval to get this in. I have no particular desire to
push this for 2.18, so trunk-only approval is fine for me if we can branch soon.
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Flags: approval? → approval+
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.17; previous revision: 1.16
done
Checking in template/en/default/admin/keywords/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/keywords/list.html.tmpl,v
 <--  list.html.tmpl
new revision: 1.5; previous revision: 1.4
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/table.html.tmpl,v
done
Checking in template/en/default/admin/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/table.html.tmpl,v 
<--  table.html.tmpl
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.