[de-xbl] Inline and Scriptify recurrence-preview binding.

RESOLVED FIXED in 6.9

Status

RESOLVED FIXED
4 months ago
27 days ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks: 1 bug)

unspecified

Details

Attachments

(1 attachment, 7 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 months ago
Assignee: nobody → arshdkhn1
Blocks: 1484976
(Assignee)

Comment 1

4 months ago
Posted patch recurrence-preview.patch (obsolete) — Splinter Review
Attachment #9028854 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

4 months ago
Attachment #9028854 - Flags: review?(mkmelin+mozilla) → feedback?(mkmelin+mozilla)
Comment on attachment 9028854 [details] [diff] [review]
recurrence-preview.patch

Review of attachment 9028854 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like it works

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul
@@ +508,5 @@
>  
>    <!-- preview -->
>    <groupbox id="preview-border" flex="1">
>      <caption id="recurrence-preview-label">&event.recurrence.preview.label;</caption>
> +    <hbox id="recurrence-preview" flex="1" style="overflow: hidden;">

put styling into the css file, though it looks like this is an unrelated error - also on trunk the months are slightly too large so the last one is cut off by some pixels.
Attachment #9028854 - Flags: feedback?(mkmelin+mozilla) → feedback+
(Assignee)

Comment 3

4 months ago
Posted patch recurrence-preview.patch (obsolete) — Splinter Review
Magnus, could you please post a screenshot of the horizontal cutting thing that you were talking about? I guess it is not related to this patch.
Flags: needinfo?(mkmelin+mozilla)
Attachment #9029955 - Flags: review?(makemyday)
Attachment #9029955 - Flags: feedback+
(Assignee)

Updated

4 months ago
Attachment #9028854 - Attachment is obsolete: true
Yeah not related to this patch, and very minor, so let's not investigate this further atm.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 5

a month ago
Posted patch recurrence-preview.patch (obsolete) — Splinter Review
Attachment #9029955 - Attachment is obsolete: true
Attachment #9029955 - Flags: review?(makemyday)
Attachment #9042785 - Flags: review?(makemyday)

Comment 6

a month ago
Comment on attachment 9042785 [details] [diff] [review]
recurrence-preview.patch

Review of attachment 9042785 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, r+ with the below comments considered. If you don't have different OS to investigate for the last comment, maybe Paenglab could help.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +22,5 @@
> +        this.mResizeHandler = null;
> +
> +        this.mDateTime = null;
> +
> +        this.mResizeHandler = this.onResize.bind(this);

Can you please remove the empty lines above.

@@ +52,5 @@
> +        // this is a simple division which always yields a positive integer value.
> +        let numHorizontal =
> +            (containerWidth -
> +                (containerWidth % contentWidth)) /
> +            contentWidth;

Can you please change this to

let cWidth = containerWidth % contentWidth;
let numHorizontal = (containerWidth - cWidth) / contentWidth;

@@ +62,5 @@
> +        // this is a simple division which always yields a positive integer value.
> +        let numVertical =
> +            (containerHeight -
> +                (containerHeight % contentHeight)) /
> +            contentHeight;

and this to

let cHeight = containerHeight % contentHeight;
let numVertical = (containerHeight - cHeight) / contentHeight;

::: calendar/base/content/dialogs/calendar-event-dialog.css
@@ +16,2 @@
>      -moz-user-focus: normal;
>  }

Remove the recurrence-preview block entirely. It's non-functional with the replacement of recurrence-preview with the box element.

@@ +63,5 @@
>  }
> +
> +#recurrence-preview {
> +    overflow: hidden;
> +}

css files in calendar/base/content are not intended to define styles but only hold the binding definitions. Styling should go into calendar/base/themes/common or if OS specific into its sibbling folder. For this definition, calendar/base/themes/common/dialogs/calendar-event-dialog.css might be an appropriate venue, since we have already a definition for ther recurrence dialog therein.

However, on Windows, this styling doesn't seem to be required (I see no difference with or without it, so you probably want to move this into a OS specific definition.
Attachment #9042785 - Flags: review?(makemyday) → review+

Comment 7

a month ago

One more thing: can you please add JSDoc descriptions to RecurrencePreview and its functions?

(Assignee)

Comment 8

a month ago
Posted patch recurrence-preview.patch (obsolete) — Splinter Review

Makemyday, I have addressed all you comments. Lemme know if you want to update some of the doc comments that I have added. The overflow property doesn't do much even on *nix OSes so removing it.

Attachment #9042785 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #9042907 - Flags: review+
(Assignee)

Comment 9

a month ago

Richard I have removed overflow:hidden css rule from recurrence-preview element. It doesn't do much on windows and macos. It just crops few pixel of content maybe 3 or 4px. wdyt about it?

Flags: needinfo?(richard.marti)
(Assignee)

Comment 10

a month ago

Richard, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

I don't know. This is too deep for me. Maybe better you ask Brian.

(Assignee)

Comment 12

a month ago

Brian, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

Flags: needinfo?(richard.marti) → needinfo?(bgrinstead)
(Assignee)

Updated

a month ago
Keywords: checkin-needed
(Assignee)

Comment 14

a month ago
Posted patch recurrence-preview.patch (obsolete) — Splinter Review
Attachment #9042907 - Attachment is obsolete: true
Attachment #9042942 - Flags: review+

Comment 15

a month ago
Comment on attachment 9042942 [details] [diff] [review]
recurrence-preview.patch

Review of attachment 9042942 [details] [diff] [review]:
-----------------------------------------------------------------

Doc blocks look ok in general, just some nit

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +27,5 @@
> +        this.mDateTime = null;
> +        this.mResizeHandler = this.onResize.bind(this);
> +        window.addEventListener("resize", this.mResizeHandler, true);
> +    },
> +    /**

Can you please add an empty line before the doc block here and befor the other blocks below?

@@ +35,5 @@
> +        window.removeEventListener("resize", this.mResizeHandler, true);
> +    },
> +    /**
> +     * Setter for mDateTime property.
> +     * @param {date} val - The date value that is to be set.

The typing is not accurate here - the argument is not a JS date but an calIDateTime object.

@@ +42,5 @@
> +        this.mDateTime = val.clone();
> +        return this.mDateTime;
> +    },
> +    /**
> +     * Getter for mDateTime property.

Add a @return value here - type as per comment above.

Updated

a month ago
Status: NEW → ASSIGNED

(In reply to Arshad Khan [:arshad] from comment #12)

Brian, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

It's not ringing a bell for me. Paolo, do you remember anything about this?

Flags: needinfo?(bgrinstead) → needinfo?(paolo.mozmail)

Comment 17

a month ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/542023c45213
Inline and Scriptify recurrence-preview binding. r=MakeMyDay

Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

a month ago
Target Milestone: --- → 6.9

Comment 18

a month ago

Arshad, can you please still fix the nits in comment 15?

Flags: needinfo?(arshdkhn1)
(Assignee)

Comment 19

a month ago
Posted patch recurrence-preview.patch (obsolete) — Splinter Review
Attachment #9042942 - Attachment is obsolete: true
Flags: needinfo?(arshdkhn1)
Attachment #9043197 - Flags: review+
(Assignee)

Comment 20

a month ago
Posted patch recurrence-preview.patch (obsolete) — Splinter Review
Attachment #9043197 - Attachment is obsolete: true
Attachment #9043198 - Flags: review+
(Assignee)

Comment 21

a month ago
Attachment #9043198 - Attachment is obsolete: true
Attachment #9043199 - Flags: review?(makemyday)

Comment 22

a month ago

(In reply to Brian Grinstead [:bgrins] from comment #16)

(In reply to Arshad Khan [:arshad] from comment #12)

Brian, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

It's not ringing a bell for me. Paolo, do you remember anything about this?

The default is "-moz-user-focus: ignore;", and I think you need to still set it to "normal" if you want the element to be accessible with the keyboard.

Flags: needinfo?(paolo.mozmail)

Comment 23

27 days ago
Comment on attachment 9043199 [details] [diff] [review]
rp-doc-nits.patch

Review of attachment 9043199 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, r+ with this fixed

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +39,4 @@
>      /**
>       * Setter for mDateTime property
> +     * @param {calIDateTime object} val - The date value that is to be set.
> +     * @returns {calIDateTime object} - mDateTime that is cloned value of original val argument.

Make it just

* @param   {calIDateTime} val  The datetime value that is to be set.	
* @returns {calIDateTime}      A clone of the passed in argument.
Attachment #9043199 - Flags: review?(makemyday) → review+
You need to log in before you can comment on or make changes to this bug.