Closed Bug 377401 Opened 13 years ago Closed 13 years ago

[Proto] Always 3 minimonths in Recurrence-dialog

Categories

(Calendar :: Internal Components, defect)

Lightning 0.3.1
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: berend.cornelius09, Assigned: berend.cornelius09)

Details

Attachments

(1 file, 1 obsolete file)

In the Recurrence dialog should initially appear 3 minimonths in the preview section
Summary: Always 3 minimonths in Recurrence-dialog → [Proto] Always 3 minimonths in Recurrence-dialog
Attachment #261472 - Flags: first-review?(michael.buettner)
Comment on attachment 261472 [details] [diff] [review]
added two other minimonths to preview

>             <xul:row anonid="row">
>               <xul:recurrence-calendar anonid="minimonth"/>
>+               <xul:recurrence-calendar anonid="minimonth"/>
>+              <xul:recurrence-calendar anonid="minimonth"/>
>               <xul:spacer/>
>             </xul:row>
Please align the new elements with the recurrence-calendar above.

>+            <field name="mbStartUp">false</field>
What do you need this flag for?

>             <constructor>
>               <![CDATA[
>                 var self = this;
>                 this.mResizeHandler = function resizeHandler() { self.onResize(); };
>-                window.addEventListener("resize", this.mResizeHandler, true);
>+                window.addEventListener("resize", this.mResizeHandler, true); 
>+                this.onResize();
Is there any reason you explicitly calling onResize() here? The appropriate handler will be called anyway, isn't it so?

>-          var numHorizontal = (containerWidth-(containerWidth%contentWidth))/contentWidth;
>+	  var numHorizontal = 2
>+	  if (!this.mbStartUp){
>+          	numHorizontal = (containerWidth-(containerWidth%contentWidth))/contentWidth;
>+          }
>+	  else{
>+            this.mbStartUp = false;
>+	  }
I don't understand what mbStartUp stands for? The state of this flag is never changed...

>         <!-- recurrence pattern -->
>-        <xul:groupbox>
>+        <xul:groupbox class="recurrence-groupbox">
What is this new class for?

>-        <xul:groupbox>
>+        <xul:groupbox class="recurrence-groupbox">
Same question here.

>         <!-- preview -->
>-        <xul:groupbox flex="1">
>+        <xul:label class="recurrence-groupbox" value="&event.recurrence.preview.label;"/>
>+	<xul:recurrence-preview id="recurrence-preview" class="recurrence-groupbox" flex="1" /> 
>+<!--        <xul:groupbox flex="1">
>           <xul:caption label="&event.recurrence.preview.label;"/>
>           <xul:recurrence-preview id="recurrence-preview" flex="1"/>
>-        </xul:groupbox>
>+        </xul:groupbox>  -->
Please remove the comment here as it doesn't explain anything.

>-.datepicker-box {
>+datepicker-box {
> 	-moz-binding: url("chrome://calendar/content/sun-calendar-event-dialog-recurrence-datepicker.xml#datepicker-box");
> }
Why do you change the datepicker-box class? Is this relevant for this patch?

>-  -moz-padding-start: 8px;
>-  -moz-padding-end: 10px;
Why do you remove the padding here?

>-  -moz-padding-start: 8px;
>-  -moz-padding-end: 10px;
Same question here.

>-.minimonth-day[budy="true"] {
>+.minimonth-day[busy="true"] {
Nice catch...
Attachment #261472 - Flags: first-review?(michael.buettner) → first-review-
Attached patch second try...Splinter Review
Attachment #261472 - Attachment is obsolete: true
Attachment #261777 - Flags: first-review?(michael.buettner)
Comment on attachment 261777 [details] [diff] [review]
second try...

>-                window.addEventListener("resize", this.mResizeHandler, true);
>+                window.addEventListener("resize", this.mResizeHandler, true); 
just a style nit, there an additional space at the end of this line.

>-          <xul:grid style="min-width: 28em">
>+          <xul:grid >
again, just a style nit, there's an additional space before the '>'.

>-.minimonth-day[budy="true"] {
>+.minimonth-day[busy="true"] {
although this change is correct, it is not related to this bug. furthermore, it prevents this patch to go in before 0.5 has been shipped. please feel free to submit a new patch without this change, otherwise I'll check this in after 0.5 has been released.

r=mickey, i'll change the style nits before checking this in. thanks for the patch.
Attachment #261777 - Flags: first-review?(michael.buettner) → first-review+
Whiteboard: [checkin needed after 0.5]
patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
OS: Solaris → All
Hardware: Sun → All
Whiteboard: [checkin needed after 0.5]
Target Milestone: --- → 0.7
verified with Lightning build 2007070304
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.