Comment on attachment 9097684 [details] [diff] [review] Bug-1568660_remove-grid-calendar-properties-dialog.patch Review of attachment 9097684 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/content/dialogs/calendar-properties-dialog.xul @@ +35,5 @@ > + <script src="chrome://lightning/content/lightning-utils.js"/> > + <script src="chrome://lightning/content/lightning-calendar-properties.js"/> > + <script src="chrome://lightning/content/caldav-lightning-utils.js"/> > + <script src="chrome://lightning/content/caldav-lightning-calendar-properties.js"/> > + <script src="chrome://gdata-provider/content/gdata-calendar-properties.js"/> note bug 1584614, so just let gdata be @@ +44,4 @@ > label="&calendarproperties.enabled.label;" > oncommand="setupEnabledCheckbox()"/> > > + <html:table id="calendar-properties-grid"> maybe change id to calendar-properties-table @@ +51,5 @@ > disable-with-calendar="true" > control="calendar-name"/> > + </html:th> > + <html:td> > + <hbox flex="1" class="input-container"> don't think you should have an <hbox> in here @@ +54,5 @@ > + <html:td> > + <hbox flex="1" class="input-container"> > + <html:input id="calendar-name" > + class="input-inline" > + disable-with-calendar="true"/> please set aria-labelled-by, here and elsewhere while you're here. and add input type @@ +92,5 @@ > + </html:th> > + <html:td> > + <hbox flex="1" class="input-container"> > + <!-- XXX Make location field readonly until Bug 315307 is fixed --> > + <html:input id="calendar-uri" type="url" @@ +148,5 @@ > + <html:td colspan="2"> > + <spacer/> > + <hbox id="no-identity-notification" class="notification-inline" flex="1"> > + <!-- notificationbox will be added here lazily. --> > + </hbox> I don't think this is the right place to have a notification. Separate table for the identity stuff, and the notification above? ::: calendar/base/themes/common/dialogs/calendar-properties-dialog.css @@ +2,4 @@ > * 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/. */ > > + @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); I don't think we want to add more default xul namespacing @@ +14,2 @@ > min-height: 26px; > } please switch to 2 space indent ::: calendar/lightning/content/lightning-utils.js @@ +22,4 @@ > */ > function ltnInitMailIdentitiesRow() { > if (!gCalendar) { > + document.getElementById("calendar-email-identity-row").toggleAttribute("hidden", "true"); true @@ +27,5 @@ > > let imipIdentityDisabled = gCalendar.getProperty("imip.identity.disabled"); > + document > + .getElementById("calendar-email-identity-row") > + .toggleAttribute("hidden", imipIdentityDisabled && "true"); .toggleAttribute("hidden", imipIdentityDisabled; ::: calendar/providers/caldav/content/caldav-lightning-utils.js @@ +30,4 @@ > } else { > document > .getElementById("calendar-force-email-scheduling-row") > + .toggleAttribute("hidden", "true"); true, not "true"

Comment on attachment 9098819 [details] [diff] [review] Bug-1568660_remove-grid-calendar-properties-dialog-1.patch Review of attachment 9098819 [details] [diff] [review]: ----------------------------------------------------------------- The dropdowns for resfresh calendar and email are a bit wider than the other fields (maybe 10px or so). Please fix that. Other than that, looks ok to me ::: calendar/base/themes/common/dialogs/calendar-properties-dialog.css @@ +9,3 @@ > } > > +html|table#calendar-properties-table html|th, You can remove the html|table preable from the id selector (it's just slower)

Comment on attachment 9098878 [details] [diff] [review] Bug-1568660_remove-grid-calendar-properties-dialog-2.patch Review of attachment 9098878 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, and nice to remove the overlays. There's one little thing I see to change in the CSS file. I think the notification part could use some work: 1. I see that Magnus disagrees, but I don't think it works as well at the top of the dialog, because it pushes everything else down when it appears, making things feel jumpy. I think it works better just above the email drop down where it was. Or maybe below (then the string would have to change because it mentions "below"). Since there's disagreement, it's a good question for Alessandro. 2. When you dismiss the notification, there's still empty space where it was. Previously the space would go away. Another little nit-picky optional thing: in TB 60 if you resized the dialog, the fields remained in place at the top of the window, with empty space expanding below them. In the new one the space expands above the fields/table so they move around. Assuming it's easy to fix, it would be better to retain the previous behavior. Here's an edge case I noticed that's probably not worth solving in this bug: If you select "None" more than once in the email drop down, more than one notification appears. In TB 60 more than one would actually appear, but the new ones would hide the previous ones so it still looked like there was only one (until you had to click close more than once to get rid of all of them). Probably best fixed in a follow-up. ::: calendar/base/themes/common/dialogs/calendar-properties-dialog.css @@ +8,5 @@ > + margin-inline-start: 20px; > +} > + > +#calendar-properties-table html|th, > +#calendar-properties-table html|td { I think we can remove these `html|` prefixes. When I removed them the styles were still being applied and everything looked fine. Unless I'm missing something.

Comment on attachment 9099001 [details] [diff] [review] Bug-1568660_remove-grid-calendar-properties-dialog-3.patch Review of attachment 9099001 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Notification at the very top works well. Multiple notifications are stacked vertically. Space goes away when you dismiss a notification. Resizing the dialog leaves the inputs at the top. A couple of potential follow-ups for the full polish treatment (priority would likely be low relative to other things): 1. a notification can push the ok/cancel buttons down partly below the bottom of the window, would be better if they always stayed fully visible. 2. selecting "none" for email more than once creates multiple identical notifications, would be better to not duplicate these.

Comment on attachment 9099001 [details] [diff] [review] Bug-1568660_remove-grid-calendar-properties-dialog-3.patch Review of attachment 9099001 [details] [diff] [review]: ----------------------------------------------------------------- I'll fix the issues mentioned below before landing. ::: calendar/base/themes/common/dialogs/calendar-properties-dialog.css @@ +30,5 @@ > +#calendar-refreshInterval-menulist, > +#email-identity-menulist { > + flex: 1; > + margin-inline-end: 7px; > +} \ No newline at end of file No newline at the end of the file, it looks like this in the patch: \ No newline at end of file ::: calendar/lightning/content/lightning-utils.js @@ +27,5 @@ > > let imipIdentityDisabled = gCalendar.getProperty("imip.identity.disabled"); > + document > + .getElementById("calendar-email-identity-row") > + .toggleAttribute("hidden", imipIdentityDisabled); This gave a linting error.