Closed Bug 244265 Opened 16 years ago Closed 16 years ago
Abstract out the typical admin page table to a separate template
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.
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)
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
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-
URI fixed in interface comments.
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><none></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.
Target Milestone: --- → Bugzilla 2.20
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
You need to log in before you can comment on or make changes to this bug.