Closed Bug 1569925 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Assignee: nobody → khushil324
Attachment #9081601 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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.
Attachment #9081601 - Flags: feedback?(mkmelin+mozilla) → feedback-
Attachment #9081601 - Attachment is obsolete: true
Attachment #9081940 - Flags: feedback?(mkmelin+mozilla)
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)
Attachment #9081940 - Flags: feedback?(mkmelin+mozilla) → feedback-

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

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

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

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

Attachment #9081940 - Attachment is obsolete: true
Attachment #9082878 - Flags: feedback?(mkmelin+mozilla)
Attachment #9082878 - Attachment is obsolete: true
Attachment #9082878 - Flags: feedback?(mkmelin+mozilla)
Attachment #9082882 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9082882 - Flags: feedback?(mkmelin+mozilla) → feedback-

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

Attachment #9082882 - Attachment is obsolete: true
Attachment #9083416 - Flags: feedback?(mkmelin+mozilla)
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?

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

are all the flex="1" necesary?

Yess.

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

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

Looks good now.
Attachment #9083416 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attachment #9083416 - Flags: review?(philipp)
Attachment #9083416 - Flags: review?(philipp) → review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a5d2ecd85991
remove grid usage from datetimepickers.js. r=philipp

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.