Add Calendar and Task toolbar for tabs on top

RESOLVED FIXED in 1.3

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

(Depends on 1 bug)

unspecified
Dependency tree / graph

Details

Attachments

(7 attachments, 10 obsolete attachments)

63.92 KB, image/png
Fallen
: feedback+
Details
69.15 KB, image/png
andreasn
: ui-review+
Details
39.21 KB, image/png
Details
3.10 KB, application/zip
Details
1.78 KB, image/png
Details
453.35 KB, patch
Paenglab
: review+
andreasn
: ui-review+
Details | Diff | Splinter Review
448.07 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
In bug 644169 Thunderbird will move to using tabs on top. This means there will be no toolbar in Calendar & Task Modes. We should add a toolbar here and can run riots on adding toolbar buttons!

We should figure out what buttons make sense here and add them in. The minimal set I'd go with is:

Calendar Tab: Synchronize|New Event|New Task|Edit|Delete|Go To Today|Print
     Default:     Y      |    Y    |   Y    | Y  |  Y   |    N      |  N

Task Tab    : Synchronize|New Event|New Task|Edit|Delete|Print
     Default:     Y      |    Y    |   Y    | Y  |  Y   |  N


Possible suggestions for further buttons are:

Common:
* Convert To (Message/Task/Event) - bug 575413
* Show Today Pane
* Import/Export related buttons
* Subscribe / New Calendar

Calendar Mode:
* Calendar Views (As Dropdown or Single buttons)
* Next/Previous View
* Find Events (Maybe as a Tri-state, see bug 392083)
* Rotate View
* Attendance (Decline/Accept/Tentative)

Task Mode:
* Mark Completed / Category / Priority (same as in task header)

I'd appreciate if someone UI-affine could look over these and decide which buttons to take. I'd also appreciate if someone could take over the CSS styling for the patch I have coming up.
Depends on: tb-tabsontop
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
This is the v1 patch that adds the toolbars (but does not style them). I'd appreciate if someone could add the css styles for this patch.
Attachment #580737 - Flags: feedback?(richard.marti)
Posted patch Some style fixes to patch above (obsolete) β€” β€” Splinter Review
This adds the gradient and an initial stab at the icons (these behaves weird for me). Only gnomestripe for now, but other themes coming.
Comment on attachment 580737 [details] [diff] [review]
Fix - v1

This approach looks promising and with the changes in the tot bug to give a special class for the toolbox makes it easier to style.
Attachment #580737 - Flags: feedback?(richard.marti) → feedback+
Posted patch Add icons to all buttons (obsolete) β€” β€” Splinter Review
This patch gives for every button an icon for big and small buttons. For Mac and Windows I used a already existing icon for 'Synchronize'. Philipp and Andreas, what do you mean to this icon?

I gave only to 'Edit', 'Delete' and 'Print' a disabled state in CSS. Could the other buttons also have a disabled state?

With the rules in lightning.css no icons are present in customize window. With moving them to lightning-toolbar.css this problem would be solved. But this file is a common file. Would it be okay to move this file to the themes? But then a solution should be found how the Aero icons can be defined (my solution is bug 684518).

Andreas; following monochrome icons are missing for Aero: 'Synchronize', 'New Task', 'New Event' and 'Go to Today'. Please can you create this icons?
Attachment #582590 - Flags: feedback?(philipp)
Comment on attachment 582590 [details] [diff] [review]
Add icons to all buttons

(In reply to Richard Marti [:paenglab] from comment #4)
> Created attachment 582590 [details] [diff] [review]
> Add icons to all buttons
> 
> This patch gives for every button an icon for big and small buttons. For Mac
> and Windows I used a already existing icon for 'Synchronize'. Philipp and
> Andreas, what do you mean to this icon?
I believe andreasn wanted to create a new icon here.

> I gave only to 'Edit', 'Delete' and 'Print' a disabled state in CSS. Could
> the other buttons also have a disabled state?
Unless its too much extra work, I think we should have the icons there, just in case.


> With the rules in lightning.css no icons are present in customize window.
> With moving them to lightning-toolbar.css this problem would be solved. But
> this file is a common file. Would it be okay to move this file to the
> themes? 
Sure, go ahead


> But then a solution should be found how the Aero icons can be
> defined (my solution is bug 684518).
Yeah, I should look into that bug. I'll give feedback on that asap so you can make use of it.


> Andreas; following monochrome icons are missing for Aero: 'Synchronize',
> 'New Task', 'New Event' and 'Go to Today'. Please can you create this icons?




Regarding feedback on the patch, I'd suggest to create a common rule that sets the right list-style-image for the calendar toolbars so you don't have to repeat this for every button.

Also, regarding the -moz-any rule, why do we need a separate rule for Lightning? Doesn't Thunderbird already provide a rule that uses .toolbarbutton-1 and sets those widths? Otherwise, wouldn't it make sense to add an additional class for the toolbar buttons?

You have a mail theme change in the patch too, I believe this needs to go in the mail tabs on top patch?
Attachment #582590 - Flags: feedback?(philipp) → feedback+
(In reply to Philipp Kewisch [:Fallen] from comment #5)
> Comment on attachment 582590 [details] [diff] [review]
> Add icons to all buttons
> 
> (In reply to Richard Marti [:paenglab] from comment #4)
> > Created attachment 582590 [details] [diff] [review]
> > Add icons to all buttons
> > 
> > This patch gives for every button an icon for big and small buttons. For Mac
> > and Windows I used a already existing icon for 'Synchronize'. Philipp and
> > Andreas, what do you mean to this icon?
> I believe andreasn wanted to create a new icon here.

The existing icon on Linux reminds me to much to the Firefox Synch and maybe TB becomes also this synch functionality then we have two icons with diferent functions. The icon I used was as I remember correct used under TB 2 for 'Synchronise Calendar'. It exists a second icon with a globe instead of a calendar. May this be better for this function?

> Regarding feedback on the patch, I'd suggest to create a common rule that
> sets the right list-style-image for the calendar toolbars so you don't have
> to repeat this for every button.

Good I'll do this.

> Also, regarding the -moz-any rule, why do we need a separate rule for
> Lightning? Doesn't Thunderbird already provide a rule that uses
> .toolbarbutton-1 and sets those widths? Otherwise, wouldn't it make sense to
> add an additional class for the toolbar buttons?

The TB rule is only for the TB buttons. I can add a new class but then I hope Add-on developers don't use this class when they don't have 18px icons.

> You have a mail theme change in the patch too, I believe this needs to go in
> the mail tabs on top patch?

This came from Andreas's patch. I had no actual build to check if this is still needed.
Posted patch Full patch without some icons (obsolete) β€” β€” Splinter Review
Here is my Christmas gift ;)

Still the missing icons as in comment 4 noted.

Mac has now the correct icon size for the toolbar in #mail-toolbox and the tabmail-buttons have now 24px icons.

Under Aero the tabmail-buttons have now the same hover appearance like the all-tabs button or the scroll-arrows.

With a persona applied the today-pane sidebarheader has the same gradient as the toolbox. Before the today-pane had some parts where also the personas was applied. Please give a feedback if this is okay like this.
Attachment #580737 - Attachment is obsolete: true
Attachment #581996 - Attachment is obsolete: true
Attachment #582590 - Attachment is obsolete: true
Attachment #584265 - Flags: feedback?(philipp)
The screenshot shows how it looks under Mac with a Personas applied. Are the big icons for Calendar and Tasks okay?

Also good to see are the bigger tabmail-buttons icons and the sidebarheader with the gradient.
Attachment #584266 - Flags: ui-review?(nisses.mail)
Attachment #584266 - Flags: feedback?(philipp)
Under XP I have the problem with the wide splitter. I applied also the gradient to the splitter. Now only the splitter borders are still visible. What do you think, is this okay like this?
Attachment #584267 - Flags: ui-review?(nisses.mail)
I forgot to mention, when I set the calendar toolbar to big icons and the tasks toolbar to small, or vice versa, the smaller toolbar has then the same height as the bigger one.
Comment on attachment 584265 [details] [diff] [review]
Full patch without some icons

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

On mac I have the feeling we might not need the toolbar icons for Calendar/Task tab anymore, since the quickbar icon is now sufficiently large. On the other hand, I guess we could leave them in if Tb decides to get rid of allowing buttons in the quickfilter area, or users personal preference.

Patch looks good to me, I haven't had a chance to test this yet though. Given your screenshots it looks well though.

::: calendar/base/themes/gnomestripe/today-pane.css
@@ +54,5 @@
>  #today-pane-panel:-moz-lwtheme {
>    background-color: transparent;
> +}
> +
> +#today-pane-panel > * {

Hmm all subnodes of the today pane panel? Can't the today-pane-panel just have the color set and the other nodes inherit automatically? I guess you did this for a reason though.

::: calendar/base/themes/winstripe/win-aero/lightning-toolbar.css
@@ +60,5 @@
> +}
> +
> +/* Toolbar buttons */
> +
> +:-moz-any(

Same comment about the -moz-any rule, but since you are asking for feedback instead of review, I assume you're on top of it.
Attachment #584265 - Flags: feedback?(philipp) → feedback+
Comment on attachment 584266 [details]
Screenshot with patch applied under Mac

This is how it looks with the "Big icons" setting, right? I personally think the calendar icon could be a bit smaller, matching the appearance of the tasks icon.
Attachment #584266 - Flags: feedback?(philipp) → feedback+
Posted patch Patch addressing Philipp's feedback (obsolete) β€” β€” Splinter Review
(In reply to Philipp Kewisch [:Fallen] from comment #11)
> Comment on attachment 584265 [details] [diff] [review]
> Full patch without some icons
> 
> Review of attachment 584265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> On mac I have the feeling we might not need the toolbar icons for
> Calendar/Task tab anymore, since the quickbar icon is now sufficiently
> large. On the other hand, I guess we could leave them in if Tb decides to
> get rid of allowing buttons in the quickfilter area, or users personal
> preference.
> 
> Patch looks good to me, I haven't had a chance to test this yet though.
> Given your screenshots it looks well though.
> 
> ::: calendar/base/themes/gnomestripe/today-pane.css
> @@ +54,5 @@
> >  #today-pane-panel:-moz-lwtheme {
> >    background-color: transparent;
> > +}
> > +
> > +#today-pane-panel > * {
> 
> Hmm all subnodes of the today pane panel? Can't the today-pane-panel just
> have the color set and the other nodes inherit automatically? I guess you
> did this for a reason though.

When I give the background color to the today-pane-panel then the sidebarheader can't show the personas.

> ::: calendar/base/themes/winstripe/win-aero/lightning-toolbar.css
> @@ +60,5 @@
> > +}
> > +
> > +/* Toolbar buttons */
> > +
> > +:-moz-any(
> 
> Same comment about the -moz-any rule, but since you are asking for feedback
> instead of review, I assume you're on top of it.

I've now using .calbar-toolbarbutton-1 > .toolbarbutton-icon instead of the -moz-any construct.

This patch is also using a smaller calendar icon for the big Mac buttons.
Attachment #584265 - Attachment is obsolete: true
(In reply to Philipp Kewisch [:Fallen] from comment #12)
> Comment on attachment 584266 [details]
> Screenshot with patch applied under Mac
> 
> This is how it looks with the "Big icons" setting, right? I personally think
> the calendar icon could be a bit smaller, matching the appearance of the
> tasks icon.

Does this icon look better?
Yes, that icon looks better!
Anything other than ui-review and the icon needed so we can get this patch rolling? I've asked andreasn per email so we can speed this up.
Posted patch Latest patch (obsolete) β€” β€” Splinter Review
Updated after dotted treelines patch landing plus small change under Windows to use the same toolbar padding under XP and Aero.
Attachment #584327 - Attachment is obsolete: true
When trying this quickly on the mac I noticed the icons don't go dark when you press them. Linux looks good as far as I can see. Aero icons coming up!
Posted patch Calendar ToT toolbar (obsolete) β€” β€” Splinter Review
Patch with darker icons when pressing buttons under OSX.
Attachment #584771 - Attachment is obsolete: true
Posted file individual icons β€”
Here are the individual icons. Realized it might be best to fix the entire toolbar-aero piece, but that's a bunch of icons more.
Posted image combined image β€”
So, looking at the toolbar graphics, there is a lot we share with the main toolbar in Thunderbird already. Is there a reason we're duplicating things like copy, paste etc. ?
Andreas:
Thank you for the Aero icons. Please can you also create 'Synchronize' icons for Mac and XP?
Duplicate of this bug: 715142
Posted patch sync icons as patch (obsolete) β€” β€” Splinter Review
To be applied on top of Richards last patch.
Comment on attachment 584267 [details]
Screenshot with patch applied under XP

This looks good.
Attachment #584267 - Flags: ui-review?(nisses.mail) → ui-review+
Posted patch sync icons as patch v2 (obsolete) β€” β€” Splinter Review
For some silly reason, I managed to cut the end of the small toolbar by accident.
This should work better.
Attachment #586057 - Attachment is obsolete: true
Posted patch Full patch (obsolete) β€” β€” Splinter Review
Full patch for review. I've let the Aero icons on their own toolbar (toolbar-aero.png).

Under Aero the 'New Event' button in the today pane and the 'New Task' button in the task-addition-box are still using the old colored icons. Should they now also use the new monochrome icons?
Attachment #585287 - Attachment is obsolete: true
Attachment #586150 - Attachment is obsolete: true
Attachment #586175 - Flags: ui-review?(nisses.mail)
Attachment #586175 - Flags: review?(philipp)
Comment on attachment 586175 [details] [diff] [review]
Full patch

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

Looks good, r=philipp

How far down the branches is tabs on top for Thunderbird? If its already on aurora we are going to have trouble with the strings.

::: calendar/base/themes/winstripe/win-aero/lightning-toolbar.css
@@ +75,5 @@
> +#task-newevent-button {
> +  -moz-image-region: rect(0px 36px 18px 18px);
> +}
> +
> +#calendar-newtask-button,

What about disabled states for aero? Do those icons look the same?
Attachment #586175 - Flags: review?(philipp) → review+
(In reply to Philipp Kewisch [:Fallen] from comment #28)
> How far down the branches is tabs on top for Thunderbird? If its already on
> aurora we are going to have trouble with the strings.

Tabs on top is already on aurora. It landed immediately before the merge to aurora. Maybe it's best to land it only on c-c and leave aurora without toolbar. Or is it possible to ask the translators for extra work?

> What about disabled states for aero? Do those icons look the same?

On Aero the disabled state is made with reduced opacity.
I think we could justify a late-l10n for this as long as we communicate it properly. That means we should finish this asap though.
You could minimize l10n impact by not moving existing and unchanged strings from lightning.dtd to lightning-toolbar.dtd, at least in the comm-aurora version of the patch. In addition you could reuse some strings from e.g. calendar.dtd instead of adding new ones.
(In reply to Stefan Sitter from comment #31)
> You could minimize l10n impact by not moving existing and unchanged strings
> from lightning.dtd to lightning-toolbar.dtd, at least in the comm-aurora
> version of the patch. In addition you could reuse some strings from e.g.
> calendar.dtd instead of adding new ones.

Wouldn't this give problems later at the merge from comm-central to comm-aurora?

I checked the strings. 9 are new and 18 are moved from lightning.dtd. 4 of the new could be reused from other dtd files (calendar.dtd and calendar-event-dialog.dtd).

If the translators are informed about the moved strings, couldn't they only copy the strings from the old position?
Only tested on Linux and Windows so far (Mac is at the office), but looks good UI-wise so far.
We should also make the icons in in the tabs toolbar styled differently so we're in line with bug 715495, but that can be a separate bug report.
Posted patch Full patch v2 β€” β€” Splinter Review
Unbitrotted patch.

Carrying over r+ from previous patch.
Attachment #586175 - Attachment is obsolete: true
Attachment #586175 - Flags: ui-review?(nisses.mail)
Attachment #587036 - Flags: ui-review?(nisses.mail)
Attachment #587036 - Flags: review+
Comment on attachment 587036 [details] [diff] [review]
Full patch v2

As I mentioned on IRC, the icons in the quick filter toolbar looks a bit big on the Mac, but are now consistent in size with QFB and other icons. I do want to fix the quick filter icon to be sane in size and style though, but I haven't filed a bug about that yet, and I don't want this to block on that, so ui-r+ for now.
Attachment #587036 - Flags: ui-review?(nisses.mail) → ui-review+
Ok it looks like we are all set for comm-central, where I am pushing it now. Since we need this on aurora, I'd appreciate if someone (Paenglab? Decathlon? ssitter?) could modify the patch so that as many strings as possible are reused and list those strings that need to be added/changed regardless.
Pushed to comm-central changeset 5885cc4465d8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
Posted patch Patch for aurora β€” β€” Splinter Review
This is the patch for aurora.

Philipp please check if the labels pointing to other DTDs (to calendar.dtd and calendar-event-dialog.dtd) are working. For this I set r?

The labels remaining for translation are:

<!ENTITY lightning.toolbar.calendar.name "Calendar Toolbar">
<!ENTITY lightning.toolbar.calendar.accesskey "C">
<!ENTITY lightning.toolbar.task.name "Task Toolbar">
<!ENTITY lightning.toolbar.task.accesskey "T">
<!ENTITY lightning.toolbar.sync.label "Synchronize">
Attachment #587449 - Flags: review?(philipp)
Seems the toolbar integration into menus is not working, see Bug 718332.
I'm too tired to test & blog about this today, I'll see to it tomorrow. Another thing we should do, probably sooner than later is to replace the "Reload Remote Calendars" string with the new "Synchronize". Depending on the context the string is in, we might want to use "Synchronize Calendars", i.e in the calendar list context menu.

From mere inspection it seems you are doing the right thing though.
Comment on attachment 584266 [details]
Screenshot with patch applied under Mac

Seems like this review got left behind. Removing flag.
Attachment #584266 - Flags: ui-review?(nisses.mail)
Blocks: 719197
Blocks: 719198
Comment on attachment 587449 [details] [diff] [review]
Patch for aurora

The patch works fine for me on aurora, after hg problems and build problems I finally had a chance to test this in action.

I've filed bug 719198 to track the bugs for late-l10n, this one included.
Attachment #587449 - Flags: review?(philipp) → review+
Pushed to comm-aurora: <http://hg.mozilla.org/releases/comm-aurora/rev/18b9e02b159a>
Target Milestone: 1.4 → 1.3
http://hg.mozilla.org/comm-central/rev/5885cc4465d8#l14.40
For active/hover styles of the tabbar toolbar buttons ("calendar-tab-button" and "task-tab-button"), instead of supplying your own styles you could use the "toolbarbutton-1" class to pick up the default styles from Thunderbird and SeaMonkey. As it stands your styles don't exactly match either Shredder/Daily nor SeaMonkey trunk.
Blocks: 721330
Depends on: 718332
You need to log in before you can comment on or make changes to this bug.