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

RESOLVED FIXED in 6.9

Status

enhancement
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(1 attachment, 7 obsolete attachments)

Assignee

Description

7 months ago
No description provided.
Assignee

Updated

7 months ago
Assignee: nobody → arshdkhn1
Assignee

Comment 1

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

Updated

7 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

6 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

6 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

4 months 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

4 months 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

4 months ago

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

Assignee

Comment 8

4 months 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

4 months ago
Attachment #9042907 - Flags: review+
Assignee

Comment 9

4 months 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

4 months 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

4 months 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

4 months ago
Keywords: checkin-needed
Assignee

Comment 14

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

Comment 15

4 months 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

4 months 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

4 months 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
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

4 months ago
Target Milestone: --- → 6.9

Comment 18

4 months ago

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

Flags: needinfo?(arshdkhn1)
Assignee

Comment 19

4 months 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

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

Comment 21

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

Comment 22

4 months 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

4 months 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.