Closed Bug 1568660 Opened 1 year ago Closed 4 months ago

remove grid usage from comm/calendar/base/content/dialogs/calendar-properties-dialog.xul

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: khushil324, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → khushil324
Summary: remove grid usage from comm/calendar/base/content/dialogs/calendar-properties-dialog.xul → remove grid usage from comm/calendar/base/content/dialogs/calendar-properties-dialog.xul and comm/calendar/resources/content/calendarCreation.xul
Summary: remove grid usage from comm/calendar/base/content/dialogs/calendar-properties-dialog.xul and comm/calendar/resources/content/calendarCreation.xul → remove grid usage from comm/calendar/base/content/dialogs/calendar-properties-dialog.xul
Attachment #9097684 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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"
Attachment #9097684 - Flags: feedback?(mkmelin+mozilla) → feedback-

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

I don't think this is the right place to have a notification. Separate table
for the identity stuff, and the notification above?

I guess we should shift the notification above the table and keep calendar-email-identity-row and calendar-force-email-scheduling-row as they are.

I suppose, yes.

Attachment #9097684 - Attachment is obsolete: true
Attachment #9098819 - Flags: feedback?(mkmelin+mozilla)

If you have gdata calendar extension enabled, disable it first else it will show an error.

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)
Attachment #9098819 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attachment #9098819 - Attachment is obsolete: true
Attachment #9098878 - Flags: review?(paul)
Attachment #9098878 - Attachment description: Bug-1568660_remove-grid-calendar-properties-dialog.patch → Bug-1568660_remove-grid-calendar-properties-dialog-2.patch
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.
Attachment #9098878 - Flags: review?(paul) → feedback+
Flags: needinfo?(alessandro)

I don't mind the notification on top. I understand it can be annoying the fact that it pushes the whole content down, but it's a pretty important alert which affects the basic calendar functionality, so it makes sense to have it first and prominent.
The thing is that it should actually be above everything, so before the checkbox to enable the calendar. It looks really weird below that.

That notification container should also be a vbox otherwise multiple notification will stack horizontally, like it happens when you select None multiple times.
The NotificationBox lazy method is pretty picky when it comes to layout, so always be sure to test how multiple notifications look in various scenarios whenever you need to update the layout around it.

Flags: needinfo?(alessandro)
Attachment #9098878 - Attachment is obsolete: true
Attachment #9099001 - Flags: review?(paul)
Blocks: 1586371
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.
Attachment #9099001 - Flags: review?(paul) → review+
Keywords: checkin-needed

Hmm, that try run isn't very meaningful since you haven't rebased for a while and lot's of stuff broke since textboxes are now gone.

What about addressing comment #12. At least file a bug for it.

Yeah, filing it ASAP.

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.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/abee555c45e1
remove grid usage from calendar-properties-dialog.xul. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
See Also: → 1588694
You need to log in before you can comment on or make changes to this bug.