Closed
Bug 372826
Opened 15 years ago
Closed 14 years ago
Update about:license, about:buildconfig to the new style/look of about:
Categories
(Firefox :: General, enhancement)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 3.1b1
People
(Reporter: frenchfrog, Assigned: ehsan.akhgari)
References
()
Details
Attachments
(3 files, 8 obsolete files)
17.72 KB,
application/xhtml+xml
|
Details | |
8.22 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
4.40 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070305 BonEcho/2.0.0.3pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3pre) Gecko/20070305 BonEcho/2.0.0.3pre Simply update about:credits, about:license, about:buildconfig use the same style as about: See the attached file for a quick merge I have done from about:credits and about: . This would bring those pages in the 21st century ;) Reproducible: Always
Reporter | ||
Comment 1•15 years ago
|
||
Reporter | ||
Updated•15 years ago
|
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•15 years ago
|
||
Wanted, but won't block release.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
Comment 3•15 years ago
|
||
I took care of about:credits in bug 391911.
Summary: Update about:credits, about:license, about:buildconfig to the new style/look of about: → Update about:license, about:buildconfig to the new style/look of about:
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → ehsan.akhgari
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•15 years ago
|
||
Not sure whom to ask for review here... Note that the buildconfig.html.in files in toolkit and xpfe are identical.
Attachment #295280 -
Flags: review?(mano)
Assignee | ||
Comment 5•15 years ago
|
||
Assignee | ||
Comment 6•15 years ago
|
||
Attachment #295283 -
Flags: review?(gerv)
Assignee | ||
Comment 7•15 years ago
|
||
Assignee | ||
Comment 8•15 years ago
|
||
Comment on attachment 295280 [details] [diff] [review] Migrate about:buildconfig to the new style Hmmm, maybe gerv is the correct reviewer here?
Attachment #295280 -
Flags: review?(mano) → review?(gerv)
Comment 9•15 years ago
|
||
(In reply to comment #8) > (From update of attachment 295280 [details] [diff] [review]) > Hmmm, maybe gerv is the correct reviewer here? No, you had it right the first time. Mano or gavin would both work fine as a reviewer.
Comment 10•15 years ago
|
||
about:buildconfig is too ugly for the new style, it needs some heavy beating ;-)
Assignee | ||
Comment 11•15 years ago
|
||
Comment on attachment 295280 [details] [diff] [review] Migrate about:buildconfig to the new style (In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 295280 [details] [diff] [review] [details]) > > Hmmm, maybe gerv is the correct reviewer here? > > No, you had it right the first time. Mano or gavin would both work fine as a > reviewer. Oops! :">
Attachment #295280 -
Flags: review?(gerv) → review?(mano)
Assignee | ||
Updated•15 years ago
|
Attachment #295283 -
Flags: review?(gerv) → review?(mano)
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #10) > about:buildconfig is too ugly for the new style, it needs some heavy beating > ;-) about:license isn't that pretty as well, if you ask me! ;-)
Comment 13•15 years ago
|
||
Fine by me. Gerv
Updated•15 years ago
|
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]
Reporter | ||
Comment 14•15 years ago
|
||
hmm, by comment #9, ask gavin to review?
Assignee | ||
Updated•15 years ago
|
Attachment #295280 -
Flags: review?(mano) → review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Attachment #295283 -
Flags: review?(mano) → review?(gavin.sharp)
Comment 15•15 years ago
|
||
This makes about:buildconfig pretty hard to read, with the build options all squished together into that little center box. I'm not really sure this is worth fixing, the buildconfig page isn't really meant to be pretty...
Assignee | ||
Comment 16•15 years ago
|
||
What about about:license?
Comment 17•15 years ago
|
||
I don't know, seems like about:license would have the same problem. In your screenshot the "bubble" is almost as wide as the viewport, but increasing the window width doesn't increase the bubble width, so the problem is worse with wide windows. I have my window maximized on a widescreen display, for example. I'm pretty ambivalent about "prettying up" these pages.
Assignee | ||
Comment 18•15 years ago
|
||
Hmm, yes, but that's the same that other pages, such as about:credits do... And since they're all using the same CSS, that problem can be addressed for all of them by editing the CSS. However, I doubt that would apply to about: for example.
Assignee | ||
Comment 19•15 years ago
|
||
Hmm, any new thoughts on this, gavin? :-)
Comment 20•15 years ago
|
||
I don't think we should fix this until the about: page CSS is fixed to be less restrictive for wide windows.
Assignee | ||
Updated•15 years ago
|
Attachment #295280 -
Attachment is obsolete: true
Attachment #295280 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Attachment #295281 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #295283 -
Attachment is obsolete: true
Attachment #295283 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Attachment #295284 -
Attachment is obsolete: true
Assignee | ||
Comment 21•15 years ago
|
||
Wide layout for about:license and about:buildconfig based on the patch in bug 416891. If this gets r+, it should be checked in after that bug.
Attachment #303817 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 22•15 years ago
|
||
gavin: Now that bug 416891 is in, would you please take a look at the new patch (attachment 303817 [details] [diff] [review])? Thanks!
Reporter | ||
Comment 23•14 years ago
|
||
So, is the patch good?
Assignee | ||
Comment 24•14 years ago
|
||
(In reply to comment #23) > So, is the patch good? Yeah! Gavin: ping...
Assignee | ||
Updated•14 years ago
|
Attachment #303817 -
Flags: review?(gavin.sharp) → review?(dao)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch] [needs review gavin] → [has patch] [needs review dao]
Comment 26•14 years ago
|
||
dao isn't a valid reviewer for this, fyi. He can do an unofficial review, but you still need a toolkit reviewer to review this. :)
Comment 27•14 years ago
|
||
Comment on attachment 303817 [details] [diff] [review] Wide layout for about:license and about:buildconfig > <title>about:buildconfig</title> >+ <link rel="stylesheet" href="chrome://global/skin/about.css" type="text/css"> >+ <style type="text/css"> >+ th, td { >+ padding: 0 5px; >+ } >+ </style> Can you add that to the style sheet? > </head> > <body> >+<div id="aboutPageContainer" class="aboutPageWideContainer"> What do we need the extra container for? <body> should be enough... see ftp listings for example.
Assignee | ||
Comment 28•14 years ago
|
||
(In reply to comment #26) > dao isn't a valid reviewer for this, fyi. He can do an unofficial review, but > you still need a toolkit reviewer to review this. :) Thanks for the note, Reed. I'll ask Gavin as soon as I get Dão's r+. :-)
Assignee | ||
Comment 29•14 years ago
|
||
(In reply to comment #27) > (From update of attachment 303817 [details] [diff] [review]) > Can you add that to the style sheet? I could, but I prefer not to do it, because in the future some other pages which need about.css could use tables, and they would get the style they did not intend to. Anyway, if you think that's not an issue, it would be easy for me to modify the stylesheet. > > </head> > > <body> > >+<div id="aboutPageContainer" class="aboutPageWideContainer"> > > What do we need the extra container for? <body> should be enough... see ftp > listings for example. I merged the buildconfig containers with the body, but couldn't do the same for license, because the body element already had an ID.
Attachment #303817 -
Attachment is obsolete: true
Attachment #320131 -
Flags: review?(dao)
Attachment #303817 -
Flags: review?(dao)
Comment 30•14 years ago
|
||
(In reply to comment #29) > I could, but I prefer not to do it, because in the future some other pages > which need about.css could use tables, and they would get the style they did > not intend to. Anyway, if you think that's not an issue, it would be easy for > me to modify the stylesheet. If we ever use that stylesheet for another page with a table that needs a different styling, we should use classes or ids to distinguish between them. So I'd still prefer putting it in the stylesheet. > I merged the buildconfig containers with the body, but couldn't do the same for > license, because the body element already had an ID. Well, the id isn't needed if you modify the stylesheet to style body directly... You could remove the div from /toolkit/content/about.xhtml then, although it doesn't any harm if the stylesheet ignores it.
Assignee | ||
Comment 31•14 years ago
|
||
(In reply to comment #30) > If we ever use that stylesheet for another page with a table that needs a > different styling, we should use classes or ids to distinguish between them. So > I'd still prefer putting it in the stylesheet. OK, done. > Well, the id isn't needed if you modify the stylesheet to style body > directly... You could remove the div from /toolkit/content/about.xhtml then, > although it doesn't any harm if the stylesheet ignores it. Done. I also modified about.xhtml in the patch, because the extra div wasn't really needed any more.
Attachment #320131 -
Attachment is obsolete: true
Attachment #320233 -
Flags: review?(dao)
Attachment #320131 -
Flags: review?(dao)
Assignee | ||
Updated•14 years ago
|
Attachment #320233 -
Attachment is patch: true
Attachment #320233 -
Attachment mime type: application/octet-stream → text/plain
Comment 32•14 years ago
|
||
Comment on attachment 320233 [details] [diff] [review] Wide layout for about:license and about:buildconfig (v3) >Index: toolkit/themes/winstripe/global/about.css > html { > background: -moz-Dialog; > } > > body { >- margin: 0; >- padding: 0 1em; > color: -moz-FieldText; > font: message-box; >-} That padding is still needed. You need to move it to the html element along with the font (otherwise 1em becomes bigger). Note that !important in the .aboutPageWideContainer rule isn't needed anymore.
Attachment #320233 -
Flags: review?(dao) → review+
Assignee | ||
Comment 33•14 years ago
|
||
Addressed comment 32.
Attachment #320233 -
Attachment is obsolete: true
Attachment #320247 -
Flags: review?(dao)
Assignee | ||
Comment 34•14 years ago
|
||
Comment on attachment 320247 [details] [diff] [review] Wide layout for about:license and about:buildconfig (v4) Oops, r+=dao. Gavin, can you take another look at this? Dão caught a number of issues, so the patch is cleaner now, and should be easier to review.
Attachment #320247 -
Flags: review?(gavin.sharp)
Attachment #320247 -
Flags: review?(dao)
Attachment #320247 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [has patch] [needs review dao] → [has patch] [needs review gavin]
Comment 35•14 years ago
|
||
Comment on attachment 320247 [details] [diff] [review] Wide layout for about:license and about:buildconfig (v4) > html { > background: -moz-Dialog; >+ padding: 0 1em; > } > > body { >- margin: 0; >- padding: 0 1em; > color: -moz-FieldText; > font: message-box; As a wrote, the font property needs to be moved to the html element as it affects the 1em padding.
Assignee | ||
Comment 36•14 years ago
|
||
(In reply to comment #35) > As a wrote, the font property needs to be moved to the html element as it > affects the 1em padding. Yes, you're right. I didn't catch that last time.
Attachment #320247 -
Attachment is obsolete: true
Attachment #320253 -
Flags: review?(gavin.sharp)
Attachment #320247 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Attachment #320247 -
Flags: review+
Assignee | ||
Comment 37•14 years ago
|
||
Gavin, can you take a look at this one if you get the time? It's a low-priority but easy review.
Updated•14 years ago
|
Attachment #320253 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [has patch] [needs review gavin]
Comment 38•14 years ago
|
||
Ehsan: you'll increase your chances of catching a checkin-needed if you attach a "For checkin" patch that does whatever ought to be done now about the xpfe parts - I was looking for something to push in a hurry, and didn't want to take the time to figure out whether they've moved or moved repos or just disappeared.
Assignee | ||
Comment 39•14 years ago
|
||
The patch for checkin on mozilla-central.
Comment 40•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f7795cb888df
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
Comment 41•14 years ago
|
||
(In reply to comment #27) > What do we need the extra container for? <body> should be enough... see ftp > listings for example. Removing this element will brake third party themes which use it to skin the "about:" pages (like mine).
Comment 42•14 years ago
|
||
Pardal, yes it brakes current themes, but that it is true for more changes that are currently happening for FF3.1 (which is also saddening me...). But this particular change is easily accommodated for, even in a way that is compatible with the current FF3.0 release (and even FF2.0). More important is that this change finally allows us themers to also style the other about pages.
Comment 43•14 years ago
|
||
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081006 Minefield/3.1b1pre. about: pages have the new layouts.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•