Comment on attachment 9081207 [details] [diff] [review] Bug-1569552_remove-grid_calendar-summary-dialog.patch Review of attachment 9081207 [details] [diff] [review]: ----------------------------------------------------------------- Probably best to wait on bug 1562994 landing before updating this. ::: calendar/base/content/dialogs/calendar-summary-dialog.xul @@ +140,3 @@ > <label value="&read.only.title.label;"/> > + </html:td> > + <html:td style="width:100%;"> I think we shouldn't hardcode style, but use a class but, setting td width = 100% seems wrong. Does it do anything? Wouldn't that be 100% of the row if anything? @@ +337,5 @@ > </hbox> > + </html:td> > + </html:tr> > + <html:tr id="attachments-row" > + hidden="true" hidden="hidden"

Comment on attachment 9081207 [details] [diff] [review] Bug-1569552_remove-grid_calendar-summary-dialog.patch Review of attachment 9081207 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-summary-dialog.xul @@ +135,5 @@ > </hbox> > <box orient="horizontal"> > + <html:table> > + <html:tr> > + <html:td> th, same with the other "header" type values

Comment on attachment 9083912 [details] [diff] [review] Bug-1569552_remove-grid_calendar-summary-dialog-xul.patch Review of attachment 9083912 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-summary-dialog.css @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#item-general-box th { it would have made more sense to have the th match for a table th. But you don't have any other tables here(?) so plain th, td etc. should be fine. If you need to match, add a class to the <table> and use that, not the id. Anyway, with this patch things look pretty different: the <th>s text is centred, not left aligned like it used to. and also bold (but I think that actually is better than it used to) Another problem is that the content of e.g. the location value is cut off, so not fully visible. If it's long it should A) use the full width, and B) wrap to next line if needed

Comment on attachment 9084855 [details] [diff] [review] Bug-1569552_remove-grid_calendar-summary-dialog-xul.patch Review of attachment 9084855 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-summary-dialog.css @@ +7,5 @@ > + padding-inline-end: 5px; > +} > + > +td { > + width: 100%; needed? ::: calendar/base/content/dialogs/calendar-summary-dialog.xul @@ +131,5 @@ > <label value="&read.only.general.label;" class="header"/> > <separator class="groove" flex="1"/> > </hbox> > + <hbox flex="1" class="calendar-summary-box"> > + <html:table class="calendar-summary-dialog"> calendar-summary-table (this is not a dialog element) @@ +198,1 @@ > <label id="item-category" crop="end"/> the category item looks to be more indented/margin than the rest of the items (in the UI)

Comment on attachment 9086313 [details] [diff] [review] Bug-1569552_remove-grid-usage_calendar-summary-dialog-xul.patch Review of attachment 9086313 [details] [diff] [review]: ----------------------------------------------------------------- Code changes look good but I got this error in the console when I tested it, and the dialog was not displayed correctly. It worked fine with the same event without the patch. I'll upload screenshots. TypeError: statusRow.childNodes[i].getAttribute is not a function calendar-summary-dialog.js:181:41 onLoad chrome://calendar/content/calendar-summary-dialog.js:181 onload chrome://calendar/content/calendar-summary-dialog.xul:1 The event was from a read-only holiday calendar, with this URL: googleapi://YOUR-GMAIL-ADDRESS-HERE@gmail.com/?calendar=en.usa%23holiday%40group.v.calendar.google.com

Comment on attachment 9086463 [details] [diff] [review] Bug-1569552_remove-grid-usage_calendar-summary-dialog-xul.patch Review of attachment 9086463 [details] [diff] [review]: ----------------------------------------------------------------- Hm, got a similar error: TypeError: statusRowData.childNodes[i].getAttribute is not a function calendar-summary-dialog.js:182:45 onLoad chrome://calendar/content/calendar-summary-dialog.js:182 onload chrome://calendar/content/calendar-summary-dialog.xul:1 Looks like statusRowData.childNodes includes text nodes that don't have the getAttribute function, so I think we want statusRowData.children which doesn't have the text nodes. https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/children Also, I actually prefer the right-aligned th labels that you had in the previous patch. It's easier to see the connection with the data. Although I think they would look better if they were not bold text, but just |font-weight: normal;|. To me the bold text competes too much with the bold headings ("General" etc.) in terms of their visual weight.

Comment on attachment 9086504 [details] [diff] [review] Bug-1569552_remove-grid-usage_calendar-summary-dialog-xul.patch Review of attachment 9086504 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. I noticed something else, when looking at a repeating event that has multiple lines of text for the "Repeat" field. Before the patch the "Repeat" label was aligned to the top of its box. After the patch it's aligned in the center vertically, which isn't as clear. The fix may be as simple as aligning the table cell contents to the top rather than centering them vertically. Here's a calendar with repeating events: https://calendar.google.com/calendar/ical/4e2tnmgfn2ph2tvaefa8ldbuqo%40group.calendar.google.com/public/basic.ics (The good news is that the use of repeatDetails.childNodes doesn't cause any trouble.)

Comment on attachment 9086566 [details] [diff] [review] Bug-1569552_remove-grid-usage_calendar-summary-dialog-xul.patch Review of attachment 9086566 [details] [diff] [review]: ----------------------------------------------------------------- Repeat rows and everything else looks good, thanks! r+