1569925 - remove grid usage from comm/calendar/resources/content/datetimepickers/datetimepickers.js

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569925_remove-grid-datetimepickers.patch

Comment 2

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9081601 [details] [diff] [review]
Bug-1569925_remove-grid-datetimepickers.patch

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

Close, but the alignment is slightly off: e.g. 9 and 21 are not aligned (the borders) above each other.

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569925_remove-grid-datetimepickers.patch

Comment 4

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9081940 [details] [diff] [review]
Bug-1569925_remove-grid-datetimepickers.patch

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

While the code change is probably in the right direction, it doesn't work for me. Seems even more misaligned than before (I'd attach a screenshot but the ubuntu screenshot tool is terrible with popups)

Comment 5

[Magnus Melin [:mkmelin] (away until Aug 10)]

Might need to add some class and have it have min-width: 2ch, or something.

Comment 6

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #5)

Might need to add some class and have it have min-width: 2ch, or something.

It already has a CSS with min-width: 24px. I will look into it now.

Comment 7

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #4)

While the code change is probably in the right direction, it doesn't work
for me. Seems even more misaligned than before (I'd attach a screenshot but
the ubuntu screenshot tool is terrible with popups)

I checked on Linux Machine. It is working fine. Can you check again and describe the problem a little bit?

Comment 8

[Magnus Melin [:mkmelin] (away until Aug 10)]

Attachment: datepickers-misaligned.png

As you see in this screenshot, the 11 and 23 are not above one another at all. Compare to trunk.

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569925_remove-grid-datetimepickers-js.patch

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569925_remove-grid-datetimepickers-js.patch

Comment 11

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9082882 [details] [diff] [review]
Bug-1569925_remove-grid-datetimepickers-js.patch

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

Still misaligned. 
Maybe you should look into https://css-tricks.com/auto-sizing-columns-css-grid-auto-fill-vs-auto-fit/

::: calendar/resources/skin/datetimepickers.css
@@ +160,4 @@
>  
>  /* box around five minute grid */
>  
> +vbox[class="time-picker-five-minute-grid-box"] {

vbox.time-picker-five-minute-grid-box ...

same for the other places. Not sure this is the right approach though

Comment 12

[Richard Marti (:Paenglab)]

Is the "vbox" in front of the class needed? Normally the class itself should be enough.

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569925_remove-grid-datetimepickers-js.patch

Comment 14

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9083416 [details] [diff] [review]
Bug-1569925_remove-grid-datetimepickers-js.patch

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

::: calendar/resources/content/datetimepickers/datetimepickers.js
@@ +296,5 @@
> +                        <timepicker-minute class="time-picker-one-minute-class" value="0" label=":00" flex="1"></timepicker-minute>
> +                        <timepicker-minute class="time-picker-one-minute-class" value="1" label=":01" flex="1"></timepicker-minute>
> +                        <timepicker-minute class="time-picker-one-minute-class" value="2" label=":02" flex="1"></timepicker-minute>
> +                        <timepicker-minute class="time-picker-one-minute-class" value="3" label=":03" flex="1"></timepicker-minute>
> +                        <timepicker-minute class="time-picker-one-minute-class" value="4" label=":04" flex="1"></timepicker-minute>

are all the flex="1" necesary?

Comment 15

[Khushil Mistry [:khushil324]]

(In reply to Magnus Melin [:mkmelin] from comment #14)

are all the flex="1" necesary?

Yess.

Comment 16

[Magnus Melin [:mkmelin] (away until Aug 10)]

Comment on attachment 9083416 [details] [diff] [review]
Bug-1569925_remove-grid-datetimepickers-js.patch

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

Looks good now.

Comment 17

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ef0be1e93fc587e0b43f18fbda7cc466cf08c1c8

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a5d2ecd85991
remove grid usage from datetimepickers.js. r=philipp