1568321 - remove grid usage from comm/calendar/resources/content/publishDialog.xul

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568321_remove-grid-publishDialog.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9080167 [details] [diff] [review]
Bug-1568321_remove-grid-publishDialog.patch

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

Please update this to use the grid-layout.css file and classes

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568321_remove-grid-publishDialog.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9083779 [details] [diff] [review]
Bug-1568321_remove-grid-publishDialog.patch

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

Some bitrot, but... sorry to move the goal posts. I think the dialog should not have the description like it had, it's looking very old-fashioned. 
Please change the thing input to <html:input type="url" placeholder="&calendar.publish.example.url.description".> and remove the div with the description. This will also make the structure much cleaner and you don't even need grid layout.

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568321_remove-grid-publishDialog-xul.patch

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9084893 [details] [diff] [review]
Bug-1568321_remove-grid-publishDialog-xul.patch

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

::: calendar/resources/content/publishDialog.xul
@@ +18,4 @@
>          windowtype="Calendar:PublishDialog"
>          buttons="accept,cancel"
>          onload="loadCalendarPublishDialog()"
> +        persist="screenX screenY width"

Why persist the width? I'd think we don't want that.

@@ +44,5 @@
> +          <label control="publish-remotePath-textbox">&calendar.publish.url.label;</label>
> +        </hbox>
> +        <hbox flex="1">
> +          <label value=""/>
> +        </hbox>

huh? seems rather pointless, no?

@@ +49,5 @@
> +      </vbox>
> +      <vbox flex="1">
> +        <html:input id="publish-remotePath-textbox" oninput="checkURLField()"/>
> +        <html:input class="publish-example-url" type="url" value="&calendar.publish.example.url.description;"
> +                    readonly="readonly"/>

I think you misunderstood, it's the publish-remotePath-textbox input that would be type=url, and have the placeholder

@@ +56,2 @@
>      <html:progress id="publish-progressmeter" value="0" max="100"/>
> +  </vbox>  <!-- dialog-box -->

we can remove this comment, and the one befor the start too; dialogOverlay.xul is long gone

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568321_remove-grid-publishDialog-xul.patch

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: Bug-1568321_remove-grid-publishDialog-xul.patch

Made some changes.

Comment 9

[Paul Morris [:pmorris]]

Comment on attachment 9085006 [details] [diff] [review]
Bug-1568321_remove-grid-publishDialog-xul.patch

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

Looks good to me.  Nice simplification by removing the grid which isn't really needed here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7cf061e5e723
remove grid usage from publishDialog.xul. r=mkmelin,pmorris

Comment 11

[Jorg K (CEST = GMT+2)]

No try run, I hope this doesn't blow up ;-)