Arshad Khan [:arshad] Assignee
	Description • 6 years ago

Opening binding and replacing it with xul elements.

Binding link - https://searchfox.org/comm-central/source/calendar/base/content/calendar-item-bindings.xml#14

	Magnus Melin [:mkmelin]
	Comment 3 • 6 years ago

Comment on attachment 9007480 [details] [diff] [review]
calendar-caption.patch

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

Since this is calendar/ someone from there should review

::: calendar/base/content/dialogs/calendar-event-dialog-reminder.xul
@@ +48,4 @@
>      </hbox>
>    </vbox>
>  
> +  <hbox class="calendar-caption" align="center" id="reminder-details-caption">

here and elsewhere, note that the id comes first, then align the attributes

::: calendar/base/content/dialogs/calendar-summary-dialog.xul
@@ +349,5 @@
>  
>    <!-- Description -->
>    <box id="item-description-box" hidden="true" orient="vertical" flex="1">
>      <spacer class="default-spacer"/>
> +    <hbox class="calendar-caption" align="center">

while you're here, for the hboxes you're adding, if they are missing an id you could go ahead and add one

	Arshad Khan [:arshad] Assignee
	Comment 5 • 6 years ago

Comment on attachment 9008721 [details] [diff] [review]
calendar-caption.patch

hey check the id values of the calendar captions and also flag someone who is right person to review from calendars team.

	Magnus Melin [:mkmelin]
	Comment 6 • 6 years ago

Comment on attachment 9008721 [details] [diff] [review]
calendar-caption.patch

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

nit: in the commit message "Bug 1489791 - ", not "Bug 1489791: "

You can flag MakeMyDay or Philipp for review

::: calendar/base/content/dialogs/calendar-event-dialog-reminder.xul
@@ +104,5 @@
>  
> +  <hbox class="calendar-caption" id="reminder-actions-caption" align="center">
> +    <label value="&reminder.action.label;" control="reminder-actions-menulist"
> +      class="header"/>
> +    <separator class="groove" flex="1"/>

Please make id the first attribute

And align attributes beneath each other (at least when the line needs to wrap)

<label value="&reminder.action.label;" control="reminder-actions-menulist"
       class="header"/>

Goes in the other places too of course

	[:MakeMyDay]
	Comment 8 • 6 years ago

Comment on attachment 9009040 [details] [diff] [review]
calendar-caption.patch

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

Thanks, looks good, but please fix the style of the commit message when updating the reviwer for checkin to what magnus mentioned in comment 6.

	Jorg K (CEST = GMT+2)
	Comment 10 • 6 years ago

We really need a try run. I had two patches presented for landing so far that caused test failures:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=28496a4ad1127d65ad7a31c3a19db77b476137d9

	Jorg K (CEST = GMT+2)
	Comment 11 • 6 years ago

Try looks OK, the failure we see is pre-existing.

	Pulsebot
	Comment 12 • 6 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d197d42b67d8
Remove calendar-caption binding. r=MakeMyDay