Closed
Bug 139011
Opened 22 years ago
Closed 21 years ago
Improve buglist colors further
Categories
(Bugzilla :: Query/Bug List, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: hwaara, Assigned: burnus)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
6.04 KB,
patch
|
justdave
:
review+
|
Details | Diff | Splinter Review |
One thing that would be great if it was added to bugzilla (at least on mozilla.org), was if resolved bugs would get a "greyed-out" sort of "disabled" color; just like critical or blocker bugs become sparkling red. :-) I have a bookmark, "Today's bugs", and when I go to that query, I often see some new major bugs, some already resolved (as invalid or duplicate or whatever), and as you can see, it would be really useful if I could quickly note (by color) when a bug is already resolved.
Comment 1•22 years ago
|
||
The template in CVS does the following: <tr class="bz_[% bug.severity %] bz_[% bug.priority %] [%+ "bz_secure" IF (bug.groupset && !usebuggroups) %]"> This bug would therefore be a request to add bz_[% bug.status %] to this list. Apart from the bulking of the HTML, I don't see why we shouldn't. Gerv
Reporter | ||
Comment 2•21 years ago
|
||
So if this is really easy, can't someone who has the power Just Do It? :-)
A patch to implement the fix as described by Gerv in comment#1. Notes about this patch: 1) Blocking bug#130276 still applies, which is why I'm not going to request a review of this patch (i.e. thought is needed before adding more css classes) 2) This is inconsistent with the change in bug#100639, another indication that bug#130276 is a good thing 3) This patch suffers from the same problem as described in bug#207044 comment#7 (spaces in bug status would cause css to not work properly) 4) If the status is not included in your buglist, then the code should either a) check for this, and not include an empty 'bz_' css class, or b) always get the status from the DB, so the css class can be specified, even if it is not a displayable column. The current severity and priority css classes suffer from this as well, I think
Assignee | ||
Updated•21 years ago
|
Attachment #129859 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Attachment #129859 -
Flags: review+
Assignee | ||
Comment 4•21 years ago
|
||
> 1) Blocking bug#130276 still applies, which is why I'm not going to request a > review of this patch (i.e. thought is needed before adding more css classes) In my opinion this bug should block bug 130276 (a meta bug) not the other way round. > 2) This is inconsistent with the change in bug#100639, another indication that > bug#130276 is a good thing Hmm, I think they are quite independent. > 3) This patch suffers from the same problem as described in bug#207044 > comment#7 (spaces in bug status would cause css to not work properly) I added a css_quote filter, which changes ' ' to '_' and css encodes the other characters which might appear (like a e.g. '&'). > 4) If the status is not included in your buglist, then the code should either > a) check for this, and not include an empty 'bz_' css class, or b) always get > the status from the DB, so the css class can be specified, even if it is not a > displayable column. My patch always gets the data from the database, I regards this by the way as better solution than a. See also bug 213079 which fixed this for severity or priority. (This is the reason for making attachment 129859 [details] [diff] [review] as obsolete.)
Attachment #129859 -
Attachment is obsolete: true
Assignee | ||
Comment 5•21 years ago
|
||
After testing (http://www.physik.fu-berlin.de/~tburnus/test2/) the css_quote part and after re-reading bug#207044 comment#7, I'm not sure whether css_quote works or not. I think it should do url_quote + replacing ' ' by '_'. Moreover Mozilla doesn't seem to be able to handle those, see bug 216293. I think I should use this: +sub css_quote { + my ($toencode) = (@_); + $toencode =~ s/ /_/g; + return url_quote($toencode); +}
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Comment 6•21 years ago
|
||
Attachment #129867 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #129904 -
Flags: review?(bbaetz)
Comment 8•21 years ago
|
||
Comment on attachment 129904 [details] [diff] [review] v2.1: Fix css_quota, it now actually works with ® names, ' ' => '_' > > Quotes characters so that they may be included as part of a url. >+ >+=item C<css_quote($val)> >+ >+Quotes characters so that they may be used as CSS class names. Spaces >+are replaced by underscores. Doesn't it make sense to say it quotes a string? Anyway, I see there are other comments in the same line in this file, so it's a nit. >+ <tr class="bz_[% bug.bug_severity FILTER css_quote %] >+ bz_[% bug.priority FILTER css_quote %] >+ bz_[% bug.bug_status FILTER css_quote %] >+ bz_[% bug.resolution FILTER css_quote %] >+ [%+ "bz_secure" IF bug.isingroups %]"> I considered recommending using a FOREACH here and applying all possible styles to the row, but that would incur in style changes according to what columns are being displayed, which isn't a good idea. r=kiko on this change, and I think it shouldn't conflict with the current blocker bug -- it *allows* styling to be done, and doesn't change the default style in any way.
Attachment #129904 -
Flags: review+
Assignee | ||
Comment 9•21 years ago
|
||
> >+Quotes characters so that they may be used as CSS class names. Spaces
> >+are replaced by underscores.
> Doesn't it make sense to say it quotes a string? Anyway, I see there are other
> comments in the same line in this file, so it's a nit.
Well, strings sounds better, but they are quoted character by character.
I don't care, but I can change this word before checking in. (justdave?)
Status: NEW → ASSIGNED
Flags: approval?
Comment 10•21 years ago
|
||
Comment on attachment 129904 [details] [diff] [review] v2.1: Fix css_quota, it now actually works with ® names, ' ' => '_' You need to fix the template compile test to define a stub for the new css_quote filter or it's going to fail tests on tinderbox when you check it in.
Attachment #129904 -
Flags: review?(bbaetz) → review-
Updated•21 years ago
|
Flags: approval?
Assignee | ||
Comment 11•21 years ago
|
||
> You need to fix the template compile test to define a stub for the new
> css_quote filter or it's going to fail tests on tinderbox when you check it
in.
I changed the t/ directory -- but I missed to include it in the diff :-(
(I hate modified trees. They make diffing a hassle)
Attachment #129904 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #129980 -
Flags: review?
Comment 12•21 years ago
|
||
Comment on attachment 129980 [details] [diff] [review] v2.2: As v2.1 but contains also t/008filter.t Looks fine.
Attachment #129980 -
Flags: review? → review+
Assignee | ||
Updated•21 years ago
|
Flags: approval?
Comment 13•21 years ago
|
||
Comment on attachment 129980 [details] [diff] [review] v2.2: As v2.1 but contains also t/008filter.t actually, t/Support/Template.pm is what I was thinking of. But the 008filter.t was a good catch, too.
Attachment #129980 -
Flags: review-
Updated•21 years ago
|
Flags: approval?
Assignee | ||
Comment 14•21 years ago
|
||
> actually, t/Support/Template.pm is what I was thinking of. But the 008filter.t > was a good catch, too. Hmm, the latter I needed in order to pass runtests, but for whatever reason I didn't need to fix 004template.t, even without it passed. I wonder why ...
Attachment #129980 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #130027 -
Flags: review?(justdave)
Comment 15•21 years ago
|
||
Comment on attachment 130027 [details] [diff] [review] v2.3 Add stub to 004template.t r=justdave requesting a peek from Gerv due to adding a new "approved filter" to the filter test. (I think it's okay, but better to have another set of eyes)
Attachment #130027 -
Flags: review?(justdave)
Attachment #130027 -
Flags: review?(gerv)
Attachment #130027 -
Flags: review+
Comment 16•21 years ago
|
||
Seems fine, although I'd call it css_class_quote rather than css_quote. Gerv
Assignee | ||
Comment 17•21 years ago
|
||
> Seems fine, although I'd call it css_class_quote rather than css_quote.
Ok, changed. Interdiff (only -/+ lines):
- css_quote => \&Bugzilla::Util::css_quote ,
+ css_class_quote => \&Bugzilla::Util::css_class_quote ,
- css_quote
+ css_class_quote
-sub css_quote {
+sub css_class_quote {
-=item C<css_quote($val)>
+=item C<css_class_quote($val)>
- css_quote => sub { return $_ } ,
+ css_class_quote => sub { return $_ } ,
- return 1 if $directive =~ /FILTER\ (html|csv|js|url_quote|css_quote|
+ return 1 if $directive =~ /FILTER\ (html|csv|js|url_quote|css_class_quote|
- <col class="bz_[% id FILTER css_quote %]_column">
+ <col class="bz_[% id FILTER css_class_quote %]_column">
- <tr class="bz_[% bug.bug_severity FILTER css_quote %]
- bz_[% bug.priority FILTER css_quote %]
- bz_[% bug.bug_status FILTER css_quote %]
- bz_[% bug.resolution FILTER css_quote %]
+ <tr class="bz_[% bug.bug_severity FILTER css_class_quote %]
+ bz_[% bug.priority FILTER css_class_quote %]
+ bz_[% bug.bug_status FILTER css_class_quote %]
+ bz_[% bug.resolution FILTER css_class_quote %]
Attachment #130027 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #130219 -
Flags: review?(justdave)
Updated•21 years ago
|
Attachment #130027 -
Flags: review?(gerv)
Updated•21 years ago
|
Attachment #130219 -
Flags: review?(justdave) → review+
Updated•21 years ago
|
Flags: approval+
Assignee | ||
Comment 18•21 years ago
|
||
FIXED /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v new revision: 1.232; previous revision: 1.231 /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v new revision: 1.9; previous revision: 1.8 /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v new revision: 1.11; previous revision: 1.10 /cvsroot/mozilla/webtools/bugzilla/t/004template.t,v new revision: 1.26; previous revision: 1.25 /cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v new revision: 1.6; previous revision: 1.5 /cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v new revision: 1.7; previous revision: 1.6 /cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v new revision: 1.13; previous revision: 1.12
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•