581422 - Tweak attachment detail view

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Marc Schumann [:Wurblzap]]

Attachment: Patch

The patch proposes tweaks to the attachment detail view (4.0 and HEAD):
 - no monotype for detail text, because we use the standard font for normal
   text throughout Bugzilla
 - punctuation added to ease the flow of reading

Please give me your thoughts.

Comment 1

[Max Kanat-Alexander]

pyrzak: Your thoughts?

Comment 2

[Guy Pyrzak]

I personally don't feel strongly about this. I feel like adding a comma and a semicolon don't add that much.  But to be fair I'm not super into punctuation, a comma and a semicolon... as far as I can tell they don't make it more readable, but that's just me and not based on any evidence other than my gut reaction so... *shrug*. If Max or LpSolit find this to be a huge improvement sure, otherwise adding punctuation just adds more busy-ness to the screen which isn't needed IMO. I personally see the size to be part of the "phrase" that describes the file. The comma separates the file information from the person who created the file, which is 2 different sets of info. The new semicolon will appear visually right next to the dard red border, which already provides a form of punctuation if the private info appears. So, i think it's plenty readable, but i'm not super tied to that.

The font change I can understand thinking we use normal font everywhere, and consistency is important. I purposefully picked mono-space to further differentiate the text from the header since I didn't give it much spacing and I wasn't sure if the font size was enough to make it visually distinct from the description, but maybe its too much? Maybe we should use a serifed font instead of a mono-space?

So I guess I'm saying this patch doesn't make me think "wow, that's a huge improvement, why didn't i think of that". But it also doesn't make me think "oh wow that's totally not the right way to do it". So those are my thoughts, not sure if they are helpful since they are mostly indifferent.

I'm gonna cc aza on this one, maybe he feels more strongly about this than i do.

Comment 3

[Guy Pyrzak]

Comment on attachment 459826 [details] [diff] [review]
Patch

lets do it!

Comment 4

[Max Kanat-Alexander]

Comment on attachment 459826 [details] [diff] [review]
Patch

If pyrzak says ok, I say okay.

Comment 5

[Max Kanat-Alexander]

Checked in on trunk, but there is bitrot for 4.0. Could you supply a new patch for 4.0?

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified skins/standard/attachment.css
modified template/en/default/attachment/edit.html.tmpl
Committed revision 7710.

Comment 6

[Frédéric Buclin]

Do we still want it for 4.0 now that it has been released?

Comment 7

[Marc Schumann [:Wurblzap]]

Attachment: Patch for branch

I think so. Here's a branch patch.

Comment 8

[Max Kanat-Alexander]

Yeah, I'm still fine with it on the 4.0 branch, since it's so minor and it is a nice polish.

Comment 9

[Guy Pyrzak]

Comment on attachment 522245 [details] [diff] [review]
Patch for branch

Review of attachment 522245 [details] [diff] [review]:

looks good

Comment 10

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified template/en/default/attachment/edit.html.tmpl
Committed revision 7582.

Comment 11

[Marc Schumann [:Wurblzap]]

Thank you for review and check-in.