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

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569919_remove-grid-calendar-subscriptions-dialog.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9081596 [details] [diff] [review]
Bug-1569919_remove-grid-calendar-subscriptions-dialog.patch

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

The search button is "thinner" than it used to be, and not vertically centered.
The subscribe and unsubscribe buttons are too high up. The subscribe button top used to be aligned with the top of the top of the box to it's left.

Comment 3

[Magnus Melin [:mkmelin]]

At least that what I see when calling window.openDialog("chrome://calendar/content/calendar-subscriptions-dialog.xul");

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569919_remove-grid-calendar-subscriptions-dialog.patch

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569919_remove-grid-calendar-subscriptions-dialog.patch

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569919_remove-grid_calendar-subscriptions-dialog-xul.patch

Comment 7

[Paul Morris [:pmorris]]

Comment on attachment 9087168 [details] [diff] [review]
Bug-1569919_remove-grid_calendar-subscriptions-dialog-xul.patch

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

The UI looks good, same as before removing the grid.  I have a suggestion on the hbox/vbox structure though.

::: calendar/base/content/dialogs/calendar-subscriptions-dialog.xul
@@ +46,5 @@
> +               crop="end"/>
> +        <richlistbox id="subscriptions-listbox" flex="1"/>
> +      </vbox>
> +      <vbox>
> +        <label value=""/>

It would be better to avoid these empty spacer labels.  Would a structure like the following work?  If so, I think it would avoid the need for the margin-top style rule for the button.

vbox
    hbox 
        label ("Show Calendars that Contain:")
    hbox
        vbox
            text input box
        vbox
            button ("Search")
    hbox
        label ("Select Calendars to Subscribe To:")
    hbox
        vbox
            list box
        vbox
            button ("Subscribe")
            button ("Unsubscribe")

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Screenshot 2019-08-22 at 10.07.39.png

The button are not verticle aligned with the approach you suggested. I also tried it for many other places but I was coming across the same problem.

Comment 9

[Magnus Melin [:mkmelin]]

A little bit OT, but this dialog could need a bunch of redesign. It's so old-fashioned... the text says to select calendars to subscribe to, but then there are but subscribe AND unsubscribe. AND, ok + cancel buttons.

Slightly more modern, I'd expect the calendar list to show, and then checkboxes next to them showing subscribed or not, then just an OK button to save stuff. And the "progress" indicator.... ugh!

Then again, I'm not sure which providers support this so you can actually make use of the calendar search... Maybe we're just bad at detecting it too.

Comment 10

[Khushil Mistry [:khushil324]]

This is from the chat between me and Philipp: "What you could possibly do is mock it by calling the openDialog function with some parameters and hoping things work out, or possibly registering your own mock calendar search service provider. Right now I believe the only server that supports it is the WCAP server, which will be hard to find."

Comment 11

[Paul Morris [:pmorris]]

Comment on attachment 9087168 [details] [diff] [review]
Bug-1569919_remove-grid_calendar-subscriptions-dialog-xul.patch

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

(In reply to Khushil Mistry [:khushil324] from comment #8)
> The button are not verticle aligned with the approach you suggested. I also tried it for many other places but I was coming across the same problem.

Okay, thanks for the screenshot.  Good to know that approach doesn't "just work".  Since this dialog could use a redesign, let's just get remove the grid for now and open a follow-up bug for redesigning it.  Maybe by then we'll be out of the XUL woods.

Comment 12

[Alessandro Castellani [:aleca]]

This and many other dialogs need a thorough analysis and redesign.
I'm gonna open a meta bug to track ux-needed bugs and create mock-ups for all of them.
Cheers

Comment 13

[Jorg K (CEST = GMT+2)]

I think your identical CSS files will bust the packaging stage. You need to add them to allowed-dupes.mn or make them different somehow. Have you done a try run?

Can you please check when reviewing that no duplicate files are introduced.

Comment 14

[Jorg K (CEST = GMT+2)]

You can see the packaging error here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8c0e9a39b82b6f073c01563e7c561b1f93362bfe

[task 2019-08-22T19:17:38.428Z] 19:17:38 INFO - package> extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/calendar/skin/osx/calendar-subscriptions-dialog.css
[task 2019-08-22T19:17:38.428Z] 19:17:38 INFO - package> extensions/{e2fda1a4-762b-4020-b5ad-a41df1933103}/chrome/calendar/skin/windows/calendar-subscriptions-dialog.css

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/de1416c02453
remove grid usage from calendar-subscriptions-dialog.xul. r=pmorris
https://hg.mozilla.org/comm-central/rev/1a402de7a248
Follow-up: Fix allowed-dupes.mn. r=me

Comment 16

[Paul Morris [:pmorris]]

(In reply to Jorg K (GMT+2) from comment #13)

Can you please check when reviewing that no duplicate files are introduced.

Good to know about this duplicate files issue. I'll watch for that in the future.