1561530 - Change calendar list tree to a list

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Geoff Lankow (:darktrojan)]

Being a tree seriously over-complicates things (especially CSS things) considering we don't have a hierarchy of calendars, just a flat collection of them. This is a follow-up to bug 1558384 so that we can use CSS variables for calendar colours in the calendar list.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1561530-calendar-list-1.diff

Time for some fresh eyes on this. I think it's ready, and I've written tests to that effect, but I've reached the point where I've looked at it so long I'm probably missing something obvious.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1561530-calendar-mochitests-1.diff

First mochitests for calendar!

A few odd things to note:

• I've had to move some <command>s from one overlay to another so that listeners are attached properly when the overlay loader adds them to the document. Otherwise it works okay for the user, but not the tests.
• I've undone the bug 1151440 work-around, that works fine now.
• I've put some functions in the top-level of browser_calendarList.js, that could be put in head.js and reused by later tests.
• browser_tabs.js doesn't really do much, it was a proof-of-concept for the test suite, but it might as well remain.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 9077327 [details] [diff] [review]
1561530-calendar-list-1.diff

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

I'll give a ui-r+ because it looks with default theme almost like before. With dark theme it uses the wrong colours. I'll fix this in a follow-up bug.

::: calendar/base/themes/common/widgets/calendar-widgets.css
@@ +128,5 @@
> +}
> +
> +#calendar-list richlistitem {
> +  -moz-box-align: center;
> +  border-top: 2px transparent solid;

Why do you add this border? This doesn't look so good (content not centred) except on the last item which has the border on the bottom too. If you want to add the border, I propose 1px on top and bottom for all items.
If it was to add some padding inside the list, you should add it as padding in the #calendar-list.

Comment 4

[Geoff Lankow (:darktrojan)]

(In reply to Richard Marti (:Paenglab) from comment #3)

I'll give a ui-r+ because it looks with default theme almost like before.

That's what I was going for!

With dark theme it uses the wrong colours. I'll fix this in a follow-up bug.

It looks like setting color: inherit on #calendar-list does the job.

::: calendar/base/themes/common/widgets/calendar-widgets.css
@@ +128,5 @@

+}
+
+#calendar-list richlistitem {

• -moz-box-align: center;
• border-top: 2px transparent solid;

Why do you add this border? This doesn't look so good (content not centred)
except on the last item which has the border on the bottom too. If you want
to add the border, I propose 1px on top and bottom for all items.
If it was to add some padding inside the list, you should add it as padding
in the #calendar-list.

The border is for drag/drop placement, like the attachments list in the compose window (which I now see is 1px not 2). Happy to reduce it to 1px as I think the items are spaced too far apart anyway.

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Geoff Lankow (:darktrojan) from comment #4)

(In reply to Richard Marti (:Paenglab) from comment #3)

I'll give a ui-r+ because it looks with default theme almost like before.

That's what I was going for!

With dark theme it uses the wrong colours. I'll fix this in a follow-up bug.

It looks like setting color: inherit on #calendar-list does the job.

And defining the selected row background-color of the theme. :)

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 9077327 [details] [diff] [review]
1561530-calendar-list-1.diff

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

Sorry for the delay. I'll upload a patch that fixes bitrot in a short while. One thing though, would be great to consider:

::: calendar/base/content/dialogs/calendar-providerUninstall-dialog.xul
@@ +27,5 @@
>    <description id="post-name-description">&providerUninstall.postName.label;</description>
>    <description id="reinstall-note-description">&providerUninstall.reinstallNote.label;</description>
>  
> +  <richlistbox id="calendar-list"
> +               flex="1"/>

Can we make this a custom element? If we are re-using this list, and I think that makes sense for calendar lists, I think it should have a custom class.

Comment 7

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Debitrot calendar list - v2

Comment 8

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Provider for Google Calendar list

What I did notice, the calendar colors for the provider are not showing. Can you look into this before you push?

Comment 9

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #6)

Can we make this a custom element? If we are re-using this list, and I think
that makes sense for calendar lists, I think it should have a custom class.

The whole point was for it to not be a custom element. They are three lists that do very different things but happen to have similar appearance.

Comment 10

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #8)

What I did notice, the calendar colors for the provider are not showing. Can you look into this before you push?

It's working for me. Did you build after applying the patch? (I think you must have, or there'd be other issues, but given that the gdata build isn't symlinked to the source yet…)

Comment 11

[Jorg K (CEST = GMT+2)]

Hmm, no checkin-needed here yet and it's unclear whether some comments will be addressed.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1561530-calendar-list-2.diff

Comment 13

[Geoff Lankow (:darktrojan)]

Try run with this and bug 1569539. This needs to land first.

Comment 14

[Geoff Lankow (:darktrojan)]

(In reply to Richard Marti (:Paenglab) from comment #5)

With dark theme it uses the wrong colours. I'll fix this in a follow-up bug.

It looks like setting color: inherit on #calendar-list does the job.

And defining the selected row background-color of the theme. :)

I haven't addressed this last bit, I'm not totally sure I know what you want. I've seen background colours on lists done in several different ways.

Comment 15

[Richard Marti (:Paenglab)]

I can do this in a follow-up bug when you open it. ;-)

Comment 16

[Philipp Kewisch [:Fallen][☁️📆]]

What I did notice, the calendar colors for the provider are not showing. Can you look into this before you push?

It's working for me. Did you build after applying the patch? (I think you must have, or there'd be other issues, but given that the gdata build isn't symlinked to the source yet…)

I used an artifact build and mach build faster, it is possible that didn't pick up the gdata changes.

The whole point was for it to not be a custom element. They are three lists that do very different things but happen to have similar appearance.

Hm ok. It just seems like a thing that would be easily reusable, and I'm worried that if we have different implementations then changing things in one spot might be missed in the others.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4e9163b13c8f
Change calendar list tree to a list; r=Fallen, ui-r=Paenglab
https://hg.mozilla.org/comm-central/rev/730da0161d02
Tests for new calendar list and calendar properties dialog; r=Fallen

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/74fa0ab9f55e
follow-up - Add a warning about CSS reuse to prevent future mistakes; rs=comment-only

Comment 19

[Magnus Melin [:mkmelin]]

Slightly related to this, any reason to have the calendar-list-header have a twisty for expansion? The one that says v Calendar above the calendar list.

Comment 20

[Geoff Lankow (:darktrojan)]

I don't know the answer but the tasks filter above it is similarly collapsible. It's probably one of those things that somebody thought would be useful once.