Closed
Bug 477445
Opened 15 years ago
Closed 15 years ago
What is this "orientation" of which you speak?
Categories
(Calendar :: Lightning Only, defect)
Calendar
Lightning Only
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b1
People
(Reporter: philor, Assigned: philor)
Details
Attachments
(1 file)
3.18 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
I'm pretty sure the original reviewer of this code was drunk at the time: there's really no other explanation for approving of things like |<box orientation="horizontal"| or the strange and terrible |<vbox orientation="vertical"|.
Attachment #361118 -
Flags: review?(philipp)
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Comment 1•15 years ago
|
||
Comment on attachment 361118 [details] [diff] [review] Hide my shame >diff --git a/mail/base/content/messenger.xul b/mail/base/content/messenger.xul >--- a/mail/base/content/messenger.xul >+++ b/mail/base/content/messenger.xul >@@ -169,17 +169,17 @@ > > <popup id="messageIdContext"/> > > <tooltip id="folderpopup" class="folderSummaryPopup"/> > > <toolbox id="mail-toolbox" class="toolbox-top"> > </toolbox> > <tabmail id="tabmail" flex="1" panelcontainer="tabpanelcontainer"> >- <box id="tabmail-buttons" orientation="horizontal"/> >+ <hbox id="tabmail-buttons"/> > <tabpanels id="tabpanelcontainer" flex="1" class="plain" selectedIndex="0"> > <box id="mailContent" orient="vertical" flex="1"> > <box id="messengerBox" orient="horizontal" flex="1" minheight="100" height="100" persist="height"> Is there a reason, why you are not converting those two boxes to a vbox/hbox (same for the box in line 220)? And since you are touching mail/ as well, don't you need a Thunderbird reviewer here as well?
Assignee | ||
Comment 2•15 years ago
|
||
Oh, I missed them because I was only looking for the nonexistent orientation attribute, not the not really needed orient attribute. And no, I'm claiming that I can delegate to you the power to review removing an attribute that doesn't exist from an element that only exists for you to hook into it :)
Assignee | ||
Comment 3•15 years ago
|
||
Sigh. Coffee first, bug comments second. Through the fog, those looked like something in your overlay, not messenger.xul. Those two being <box>es with an orient attribute is part of the dynamic layout thing: View - Layout - Wide needs to set the orient property of a splitter to the value of the orient property on the mailContent box. While it's certainly possible that a vbox does a perfect job of having an orient property without having an orient attribute, we stole that code from Neil, who knows vastly more about XUL than I do and who did it with <box>es, so I'd want a really good reason for switching.
Updated•15 years ago
|
Attachment #361118 -
Flags: review?(philipp) → review+
Comment 4•15 years ago
|
||
Comment on attachment 361118 [details] [diff] [review] Hide my shame Looks good to me, r=philipp
Assignee | ||
Comment 5•15 years ago
|
||
http://hg.mozilla.org/comm-central/rev/f6f782376998
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Comment 6•15 years ago
|
||
Adding orient attributes to h/vbox would have been confusing, so I used a <box> as a reminder that the orientation was variable.
Comment 7•13 years ago
|
||
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in
before you can comment on or make changes to this bug.
Description
•