remove grid usage from comm/calendar/base/content/dialogs/calendar-print-dialog.xul
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: khushil324, Assigned: khushil324)
References
Details
Attachments
(3 files, 3 obsolete files)
calendar-print-dialog-without-patch-0.png
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 2•5 years ago
|
||
Comment on attachment 9080319 [details] [diff] [review] Bug-1568504_remove-grid-calendar-print-dialog.patch Review of attachment 9080319 [details] [diff] [review]: ----------------------------------------------------------------- Please update to use grid-layout.css
|
|
Assignee | |
Comment 3•5 years ago
|
||
|
|
||
Comment 4•5 years ago
|
||
Comment on attachment 9083773 [details] [diff] [review] Bug-1568504_remove-grid-calendar-print-dialog-xul.patch Review of attachment 9083773 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 5•5 years ago
|
||
Comment on attachment 9083773 [details] [diff] [review] Bug-1568504_remove-grid-calendar-print-dialog-xul.patch Review of attachment 9083773 [details] [diff] [review]: ----------------------------------------------------------------- r+ with these changes: ::: calendar/base/content/dialogs/calendar-print-dialog.xul @@ +47,5 @@ > + <html:div> > + <textbox id="title-field" > + class="padded" > + flex="1" > + onchange="refreshHtml();"/> If we are moving this to html, any reason not to use <html:input> here? @@ +51,5 @@ > + onchange="refreshHtml();"/> > + </html:div> > + <html:div class="flex-items-center"> > + <label control="layout-field" > + value="&calendar.print.layout.label;"/> Same here, can we use a html element instead? Same for similar elements. @@ +54,5 @@ > + <label control="layout-field" > + value="&calendar.print.layout.label;"/> > + </html:div> > + <html:div> > + <menulist id="layout-field"> This one might be a bit more tricky, but I think you can use <html:select> here with the right attributes.
|
|
Assignee | |
Comment 8•5 years ago
|
||
(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #5)
Same here, can we use a html element instead? Same for similar elements.
For label, we have control attribute assigned to it. HTML equivalent of the label is html:label and its control attribute equivalent is for. "for" only works on html:input and we have datepicker which is not html:input.
|
|
Assignee | |
Comment 9•5 years ago
|
||
Apply patch from bug 1563000 before testing this patch.
|
||
Comment 10•5 years ago
|
||
Comment on attachment 9086340 [details] [diff] [review] Bug-1568504_remove-grid_calendar-print-dialog-xul.patch Review of attachment 9086340 [details] [diff] [review]: ----------------------------------------------------------------- The code changes look good to me. Visually some alignment in the layout seems to be worse though. I'll attach some screenshots. Since Magnus and Philipp have r+'d earlier versions of this, and the point is to get rid of the XUL grids, I'm leaning toward landing this and then fixing the layout issues in a follow-up bug. Will needinfo Philipp for confirmation of that before finalizing my review. ::: calendar/base/content/dialogs/calendar-print-dialog.js @@ +50,5 @@ > // Use the contractid as value > + let option = document.createElementNS("http://www.w3.org/1999/xhtml", "option"); > + option.textContent = formatter.name; > + option.setAttribute("value", contractid); > + layoutList.appendChild(option); Huh, interesting that you can append an XHTML <option> inside a XUL <menulist>. (Things I've learned.)
|
|
Assignee | |
Comment 11•5 years ago
|
||
(In reply to Paul Morris [:pmorris] from comment #10)
Huh, interesting that you can append an XHTML <option> inside a XUL
<menulist>. (Things I've learned.)
No, I have changed menulist to html:select.
|
|
||
Comment 12•5 years ago
|
||
Print dialog before this patch. Note that "Print Settings" and other headings are left-aligned the same as the input labels like "Title".
|
|
||
Comment 13•5 years ago
|
||
Print dialog with this patch. Note that input labels like "Title" stick out to the left further than headings like "Print Settings". The left-alignment is off.
Philipp, should we fix this alignment in a follow-up bug or this one? Curious not just for this one but also for similar cases in the future.
|
|
||
Comment 14•5 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #11)
No, I have changed menulist to html:select.
Ah, of course, makes sense, thanks.
|
|
Assignee | |
Comment 15•5 years ago
|
||
I have updated the patch with solving alignment issue.
Can you check on Linux machine how it is coming up?
|
|
||
Comment 16•5 years ago
|
||
Comment on attachment 9086572 [details] [diff] [review] Bug-1568504_remove-grid_calendar-print-dialog-xul.patch Review of attachment 9086572 [details] [diff] [review]: ----------------------------------------------------------------- Alignment looks good here on linux, thanks for fixing it. r+
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 17•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/21657f746451
remove grid usage from calendar-print-dialog.xul. r=pmorris
|
||
Updated•5 years ago
|
|
|
||
Comment 18•5 years ago
|
||
Sorry I didn't respond here, I think my input isn't required any more? Feel free to ping me if you need anything.
Description
•