Closed
Bug 219555
Opened 21 years ago
Closed 19 years ago
'Format for Printing' page is a mess.
Categories
(Bugzilla :: User Interface, enhancement)
Bugzilla
User Interface
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: toms.baugis, Assigned: bugzilla-mozilla)
References
Details
Attachments
(2 files, 7 obsolete files)
129.08 KB,
image/png
|
Details | |
15.56 KB,
patch
|
bugzilla-mozilla
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030825
Build Identifier:
Well, if you check Print-Ready version of any bug, i think you'll agree, that it
is not quite readable. I'll attach another version of it in a second.
I think, that there is also no special reason, why the print-ready version
should use another cgi file than edit bug does.
Reproducible: Always
Steps to Reproduce:
Reporter | ||
Comment 1•21 years ago
|
||
Ignore headers - focus on the content.
Comment 2•19 years ago
|
||
I like your display. :)
Are you interested in proposing a patch?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 3•19 years ago
|
||
i would love to make a patch, still some questions need to be solved.
for initial i would propose to fix print ready template, and leave other stuff
(mentioned in bug 135595)
the problem is default values - there is no sense of displaying "Version:
unspecified" or "Priority: ---".
Still, hardcoded dealing with this is rather lame. If we do not target this
question now, i could try to make rearranged version of print-ready.
Comment 4•19 years ago
|
||
(In reply to comment #3)
> for initial i would propose to fix print ready template, and leave other stuff
> (mentioned in bug 135595)
Sure. No problem.
> the problem is default values - there is no sense of displaying "Version:
> unspecified" or "Priority: ---".
Why not? This makes perfect sense to me. These are informations meaning "the
version where this bug appeared is unknown or has not been given". I have many
bugs assigned to me; I want these informations in all cases.
I added a dependency to a bug which asks to display references to dependent bugs
too. You have them already in your beautiful "screenshot", so I guess it's not a
problem for you to include these two fields in the patch as well. ;)
Reporter | ||
Comment 5•19 years ago
|
||
first version. i am not clear about how far i can go with styling, so i din't
apply much style.
feel free to take it apart. i am acknowledged, that i'm not wrapping all lines
at 80 column - that i will fix later, together with other stuff.
also - styles currently are applied directly to table, td and th tags - that
will be changed to application to classes
Assignee: myk → toms
Status: NEW → ASSIGNED
Comment 6•19 years ago
|
||
I just applied your patch locally... The display looks good. Feel free to style
it a bit more, especially the background color of the "header" which I really
like in your "snapshot".
CC'ing myk who is the UI module owner.
Target Milestone: --- → Bugzilla 2.22
Comment 7•19 years ago
|
||
I haven't had a chance to look at the patch, but note that we're in the process
of converting all pages to structural HTML and stylistic CSS, and we should make
sure that this work is compatible with that effort and general direction.
Reporter | ||
Comment 8•19 years ago
|
||
now it runs smooth through tests.
added gray background.
somebody should really review it, because i'm not quite sure that i understood
fully what myk said.
Attachment #187202 -
Attachment is obsolete: true
Attachment #189410 -
Flags: review?
Updated•19 years ago
|
Attachment #189410 -
Flags: review?(LpSolit)
Comment 9•19 years ago
|
||
I think myk said that we should aim towards a direction where we would use only
HTML Strict + CSS style formatting.
As far as I can see your patch conforms to myk's comment.
Comment 10•19 years ago
|
||
See also bug 110152.
Comment 11•19 years ago
|
||
(In reply to comment #8)
I really like the display. A few (minor) points however:
- could you please add the URL field? Even if this page is printed, having a URL
may be useful, for reference.
- The creation date of the bug should be the last item in your gray box, instead
of being displayed in the middle of the page below the box.
- I think the assignee, reporter and QA contact should be displayed in the right
half of the gray box, at the same level than the product, component and status:
Product: Bugzilla Assignee: Toms Baugis <toms@myrealbox.com>
Component: User Interface Reporter: Toms Baugis <toms@myrealbox.com>
Status: ASSIGNED QA Contact: MattyT <mattyt-bugzilla@tpg.com.au>
Severity: enhancement
Priority: --
....
Toms, what do you think about my last suggestion?
Comment 12•19 years ago
|
||
Oh... forget my previous comment. The URL wasn't displayed because I didn't
specify one. Now I can see it. :)
About having the assignee, QA and reporter on the right, I finally wonder if we
have enough place. Maybe doing some tests could answer this question.
This left us with the creation date, which is a nit, really.
Comment 13•19 years ago
|
||
Comment on attachment 189410 [details] [diff] [review]
vertical layout, version 0.2
>Index: template/en/default/bug/show-multiple.html.tmpl
>+ <h1>
>+ [% terms.Bug %]
>+ <a href="show_bug.cgi?id=[% bug.bug_id %]">[% bug.bug_id %]</a>
>+ [% " [$bug.alias]" FILTER html IF Param("usebugaliases") AND bug.alias %]:
>+
>+ [% bug.short_desc FILTER html %]
>+ </h1>
Nit: I wonder if having the bug summary as the first item in the gray box
wouldn't be better, instead of this big title, especially for printing.
Moreover, could you please leave the IF block as:
[% IF Param("usebugaliases") AND bug.alias %]
([% bug.alias FILTER html %])
[% END %]
>+ <th>[% field_descs.bug_status FILTER html %]:</th>
>+ <td>
>+ [% bug.bug_status FILTER html %]
>+ [% " - $bug.resolution" FILTER html IF bug.bug_status == "RESOLVED"
>+ OR bug.bug_status == "VERIFIED"
>+ OR bug.bug_status == "CLOSED" %]
>+ </td>
Instead of the IF test, you could directly write this as:
>+ <th>[% field_descs.bug_status FILTER html %]:</th>
>+ <td>
>+ [% bug.bug_status FILTER html %]
>+ [%+ bug.resolution FILTER html %]
>+ </td>
This way, you don't care whether the resolution is blank or not.
>+ [% IF Param('useqacontact') %]
>+ <th>[% field_descs.qa_contact FILTER html %]:</th>
> <td>
>+ [% bug.qa_contact.identity FILTER html %]</td>
>+ [% END %]
Please leave <td> and [% bug.qa_contact.identity FILTER html %]</td> on the
same line.
Moreover, you have to add <tr></tr> tags in this IF block.
>- <tr>
>- <td colspan="4">
>- <b>Opened:</b>
>- [% bug.creation_ts FILTER time %]
>- </td>
>- </tr>
As I said in a previous comment, the creation date should be the last item.
Please don't remove it.
These comments are easy to addressed, so the next patch should be the final
one. I want to say that the display is really beautiful and looks very
professional. I love it! :)
Attachment #189410 -
Flags: review?(LpSolit)
Attachment #189410 -
Flags: review?
Attachment #189410 -
Flags: review-
Comment 14•19 years ago
|
||
The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
Comment 15•19 years ago
|
||
Toms, are you still interested in this bug? There is not a lot of work left to get it ready. And this would be a great UI improvement.
Reporter | ||
Comment 16•19 years ago
|
||
oh i am so sorry - currently i'm pretty overloaded at work. it would be great if somebody else could finish it! i hope to get back to bugzilla next year.
Updated•19 years ago
|
Assignee: toms → myk
Status: ASSIGNED → NEW
QA Contact: mattyt-bugzilla → default-qa
Assignee | ||
Comment 17•19 years ago
|
||
Toms: Do you have time to fix it? If so, I'll reassign to you. If not, I want to fix this.
(Reassigning to me first so I won't forget about this bug.)
Assignee: myk → bugzilla-mozilla
Comment 18•19 years ago
|
||
bkor, he said he was busy so go on! His last patch was pretty good, see my review in comment 13. There shouldn't be a lot of work left, except maybe unbitrotting his patch. ;)
Assignee | ||
Comment 19•19 years ago
|
||
Changes:
- unbitrotted (resolution_descs / status_descs)
- fixes nits
- only shows creation time if there are no comments (otherwise bug/comments.html.tmpl will show the creation time for the first comment)
- shows both dependson/blocked if there is at least a bug in one of them (most bugs to not have these fields, but I want both shown if at least one field is filled in)
- add Toms Baugis to contributor of show-multiple.html.tmpl (he wrote most of the patch, changing most of the file)
- get rid of h1
Attachment #131662 -
Attachment is obsolete: true
Attachment #189410 -
Attachment is obsolete: true
Attachment #208075 -
Flags: review?(LpSolit)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 20•19 years ago
|
||
Changes:
- 2 columns
- flags
- attachments
Attachment #208075 -
Attachment is obsolete: true
Attachment #209384 -
Flags: review?(LpSolit)
Attachment #208075 -
Flags: review?(LpSolit)
Comment 21•19 years ago
|
||
Comment on attachment 209384 [details] [diff] [review]
Patch v4: Bugzilla CVS 2006-01-23
This patch looks good. There are still some points to fix as discussed on IRC. I will review the updated patch.
Assignee | ||
Comment 22•19 years ago
|
||
Changes:
- passes runtests.pl
- only show keywords if the bug has them
- attachment flags joined with ", "
- space between left and right columns using css
- show rightcell next to opening date (not always shown)
- add my name to contributors
Attachment #209384 -
Attachment is obsolete: true
Attachment #209430 -
Flags: review?(LpSolit)
Attachment #209384 -
Flags: review?(LpSolit)
Comment 23•19 years ago
|
||
Here is how patch v5 looks like. I love it! :)
Comment 24•19 years ago
|
||
Comment on attachment 209430 [details] [diff] [review]
Patch v5
>Index: template/en/default/bug/show-multiple.html.tmpl
>+ [% IF bug.use_keywords && bug.keywords %]
>+ [% rightcells.push('keywords') %]
>+ [% END %]
Nit: You don't really care whether the installation has some defined keywords. What interests us here is whether the bug itself has some keywords. So [% bug.keywords %] is enough.
>+ [% IF bug.bug_file_loc %]
> <tr>
>+ <th>[% field_descs.bug_file_loc FILTER html %]:</th>
>+ <td colspan="3">
>+ [% IF bug.bug_file_loc
>+ AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
Nit: the second IF test doesn't need to verify 'bug.bug_file_loc' as you already tested it in the first IF test. You could write:
[% UNLESS bug.bug_file_loc.match("^(javascript|data)") %]
>+ [% IF !bug.longdescs.size %]
> <tr>
>+ <th>[% field_descs.creation_ts FILTER html %]:</th>
>+ <td>[% bug.creation_ts FILTER time %]</td>
>+
>+ [% PROCESS rightcell %]
> </tr>
> [% END %]
Nit: AFAIK, there is *always* at least one comment, even when 'commentonecreate' is turned off. So this block is probably useless and should be removed.
>+ [% IF flag.setter %]
>+ [% flag.setter.nick FILTER html %]:
>+ [% END %]
Nit: a flag *always* has a setter, so there is no need to check whether one is defined; you already know there is one. This test is useless. This concerns both bug and attachment flags.
>+ <br />
Nit: no need to try to be XHTML compliant.
>+[% BLOCK row %]
>+ <tr>
>+ <th>[% field_descs.${cell} FILTER html %]:</th>
>+ <td[% " colspan=3" IF fullrow %]>[% bug.${cell} FILTER html %]</td>
>+ [% PROCESS rightcell IF !fullrow %]
>+ </tr>
>+[% fullrow = 0 %]
>+[% END %]
Nit: fix the indentation of [% fullrow = 0 %] (add two whitespaces).
>+ [% IF flag.setter %]
>+ [% flag.setter.nick FILTER html %]:
>+ [% END %]
Nit: same comment as above.
>+ [% ELSIF name != "" %]
>+ <th class="rightcell">[% field_descs.${name} FILTER html %]:</th>
>+ <td>[% bug.${name} FILTER html %]</td>
>+ [% END %]
Nit: this is not enough. If name == "", you still have to add <td> </td> so that each row of the table has 4 columns. Some web browsers do not like when the number of columns is not consistent (you may have some UI bugs).
Despite I marked all my comments as nits, I would like to see an updated patch with all these nits fixed anyway. Please fix them and carry forward my r+ (I trust you).
r=LpSolit
Attachment #209430 -
Flags: review?(LpSolit) → review+
Updated•19 years ago
|
Flags: approval?
Assignee | ||
Comment 25•19 years ago
|
||
Carrying forward r+ as agreed.
> Nit: You don't really care whether the installation has some defined keywords.
> What interests us here is whether the bug itself has some keywords. So [%
> bug.keywords %] is enough.
fixed.
> >+ [% IF bug.bug_file_loc
> >+ AND NOT bug.bug_file_loc.match("^(javascript|data)") %]
>
> Nit: the second IF test doesn't need to verify 'bug.bug_file_loc' as you
> already tested it in the first IF test. You could write:
>
> [% UNLESS bug.bug_file_loc.match("^(javascript|data)") %]
Fixed. Used IF and changed the order (for clarity; do not like UNLESS + ELSE).
> Nit: AFAIK, there is *always* at least one comment, even when
> 'commentonecreate' is turned off. So this block is probably useless and should
> be removed.
Checked back until Bugzilla 2.10 sources and it is indeed the case. Thought it wasn't because the changed importxml.pl used on b.g.o had the reporter under a different id than the description. Removed it.
> >+ [% IF flag.setter %]
> >+ [% flag.setter.nick FILTER html %]:
> >+ [% END %]
>
> Nit: a flag *always* has a setter, so there is no need to check whether one is
> defined; you already know there is one. This test is useless. This concerns
> both bug and attachment flags.
Took this from existing Bugzilla sources. Checked current Bugzilla and it indeed always stores a setter plus assumed one is always stored. This code was introduced in bug 98801 between patch 30 and patch 31, but not because of a review comment. This template code exists in two more files, I'll file a new bug for that.
> Nit: no need to try to be XHTML compliant.
Fixed.
> Nit: fix the indentation of [% fullrow = 0 %] (add two whitespaces).
Fixed.
> >+ [% IF flag.setter %]
> >+ [% flag.setter.nick FILTER html %]:
> >+ [% END %]
>
> Nit: same comment as above.
Fixed.
> >+ [% ELSIF name != "" %]
> >+ <th class="rightcell">[% field_descs.${name} FILTER html %]:</th>
> >+ <td>[% bug.${name} FILTER html %]</td>
> >+ [% END %]
>
> Nit: this is not enough. If name == "", you still have to add <td> </td>
> so that each row of the table has 4 columns. Some web browsers do not like when
> the number of columns is not consistent (you may have some UI bugs).
Fixed.
Attachment #209430 -
Attachment is obsolete: true
Attachment #210842 -
Flags: review+
Assignee | ||
Comment 26•19 years ago
|
||
Said in previous comment that I changed UNLESS / ELSE order to IF / ELSE. I did do that, but didn't rediff the patch. Both versions work, but IF / ELSE is much better (clarity).
Attachment #210842 -
Attachment is obsolete: true
Attachment #210843 -
Flags: review+
Updated•19 years ago
|
Flags: approval? → approval+
Comment 27•19 years ago
|
||
Checking in skins/standard/show_multiple.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/show_multiple.css,v <-- show_multiple.css
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v <-- filterexceptions.pl
new revision: 1.62; previous revision: 1.61
done
Checking in template/en/default/bug/show-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show-multiple.html.tmpl,v <-- show-multiple.html.tmpl
new revision: 1.27; previous revision: 1.26
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•