Closed Bug 1511229 Opened 6 years ago Closed 6 years ago

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

Categories

(Calendar :: Lightning Only, enhancement)

Product:
Calendar
Component:
Lightning Only
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 7 obsolete files)

rp-doc-nits.patch
(deleted), patch
MakeMyDay
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Blocks: tb-war-on-xbl
Attached patch recurrence-preview.patch (obsolete) (deleted) — — Splinter Review
Attachment #9028854 - Flags: review?(mkmelin+mozilla)
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+
Attached patch recurrence-preview.patch (obsolete) (deleted) — — 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+
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)
Attached patch recurrence-preview.patch (obsolete) (deleted) — — Splinter Review
Attachment #9029955 - Attachment is obsolete: true
Attachment #9029955 - Flags: review?(makemyday)
Attachment #9042785 - Flags: review?(makemyday)
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+

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

Attached patch recurrence-preview.patch (obsolete) (deleted) — — 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
Attachment #9042907 - Flags: review+

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)

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.

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)
Keywords: checkin-needed
Attached patch recurrence-preview.patch (obsolete) (deleted) — — Splinter Review
Attachment #9042907 - Attachment is obsolete: true
Attachment #9042942 - Flags: review+
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.
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)

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: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.9

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

Flags: needinfo?(arshdkhn1)
Attached patch recurrence-preview.patch (obsolete) (deleted) — — Splinter Review
Attachment #9042942 - Attachment is obsolete: true
Flags: needinfo?(arshdkhn1)
Attachment #9043197 - Flags: review+
Attached patch recurrence-preview.patch (obsolete) (deleted) — — Splinter Review
Attachment #9043197 - Attachment is obsolete: true
Attachment #9043198 - Flags: review+
Attached patch rp-doc-nits.patch (deleted) — — Splinter Review
Attachment #9043198 - Attachment is obsolete: true
Attachment #9043199 - Flags: review?(makemyday)

(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 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.

Attachment

General

Creator:
Created:
Updated:
Size: