Closed Bug 388016 Opened 17 years ago Closed 17 years ago

Mode Toolbar: Visual Fine Tuning

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: chris.j.bugzilla, Assigned: sipaq)

References

Details

Attachments

(8 files, 3 obsolete files)

The Mode Toolbar should look like the Standard toolbars.

- Please add a 1px horizontal shadow border at the top
- Reduce the default height to 50px
- Add a 10px space left of the Mail icon

See: http://wiki.mozilla.org/Calendar:View_Switch_Toolbar#Mode_Switch
Flags: blocking-calendar0.7+
Also, please consider changing the button text from "Task" to "Tasks".  (This was probably a typo in the original implementation.)
Summary: Mode Toolbar Visial Fine Tuning → Mode Toolbar: Visual Fine Tuning
Assignee: michael.buettner → nobody
Component: Calendar Views → Lightning Only
OS: Windows XP → All
QA Contact: views → lightning
Hardware: PC → All
Attached patch Patch v1 (obsolete) — — Splinter Review
Taking over from Mickey as he's on vacation.

The patch itself is pretty simple. The "!important" statement is necessary at in Winstripe, since line 63-67 on mozilla/toolkit/themes/winstripe/global/toolbar.css come into play on both branch and trunk (line 67-71). 

I've also added this change to Pinstripe, to keep both themes in sync. It shouldn't do any damage, but someone with a Mac should probably take a look at this.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #272623 - Flags: review?(daniel.boelzle)
Oh and one more thing: The patch does not address the "Reduce the default height to 50px" requirement. That can only be achieved by reducing the mode toolbar icons  height from 38px to 31px, thereby reducing the overall height from 57px to 50px.
Thanks for taking over, Simon :-)
I'll crop the images and attach the to this bug.
The image map is somehow strange arranged.
Wouldn't it make more sense to habe two maps. One for small one for large?
Feel free to change the image map and attach it to this bug.
(In reply to comment #5)
> The patch does not address the "Reduce the default
> height to 50px" requirement. That can only be achieved by reducing the mode
> toolbar icons  height from 38px to 31px,

I'm not convinced that the size of the *icons* is the problem.  When I measure them, they're no taller than any of Thunderbird's toolbar icons (max size that I see anywhere is about 25 pixels for icons).  Instead, the problem seems to be that there is too much blank space (1) above the icon and (2) in between the icon and the button text.

I think when Christian refers to a max height of 50 pixels, he's referring to the size of the whole button, not to the icon.

By the way, on my Thunderbird toolbar, the height of the whole button is about 47 pixels if I use "small icons" and about 55 pixels if I use "large icons", so maybe these values should also apply to Lightning?
Also, v1 of the patch does not seem to fix the typo.  "Task" should be changed to "Tasks" for the button text of that button, unless this is a localization issue that will be fixed in a different bug.
(In reply to comment #9)
> I'm not convinced that the size of the *icons* is the problem.

I didn't realize that the blank space above and below the icon was part of the icon image itself.  Sorry, that was my ignorance.  However perhaps the other points that I mentioned remain valid.
Pete, please take a look at the link that Christian supplied. 

From that link it is clear, that the whole toolbar should be no higher than 50 px. Since the padding and the font-size of the icon text are already taken away, that only leaves the icons itself, when trying to adjust the overall height of the mode toolbar.

If you feel, that the "Task" naming is a bug (I would concur), then please file a new bug for this. I'll gladly supply a patch then. But we have a pretty strict "one issue per bug"-policy here in bugzilla, and should try to adhere to that.
So, it is done.
I've adjusted the image map, plus some icons.

The new size is now:

* 25x25px for the large icons, and
* 16x16 for the small ones

I've also adjusted the lightning.css please find it attached.

Some questions are left open:
- I have no idea why the Mail icons border, on the left hand side ,is thinner than the right.

- The hover state seems not to work.

Next time we should store the images in two separate files. This makes the calculation of the coordinates for the css file more convenient.
Attached image Resized mode toolbar images —
Attachment #272623 - Flags: review?(daniel.boelzle) → review+
(In reply to comment #13)
> I've adjusted the image map, plus some icons.
> 
> The new size is now:
> 
> * 25x25px for the large icons, and
> * 16x16 for the small ones
> 
> I've also adjusted the lightning.css please find it attached.

Thanks Christian, with the adjusted icons, the full toolbar height is now 44px.

> Some questions are left open:
> - I have no idea why the Mail icons border, on the left hand side, is 
>   thinner than the right.

It does not seem to thinner in my testing, neither in activated nor in hover state. It could just be an optical illusion.

> - The hover state seems not to work.

Well, it works here in my build. Or do you mean the hover effect over an already activated icon? That's also there, but difficult to spot. Take a closer look at the bottom of the icon, when hovering over it.

I'll supply a new patch with the adjusted css values of the icons shortly.
Attached file Patch for checkin (with new icon sizes) (obsolete) —
Attachment #272623 - Attachment is obsolete: true
Latest patch checked in after discussion/approval from Daniel.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7
Please re-compress the new .png images before checkin to reduce download size (see similar Bug 377554). For example the new added .png can be shrunk by 20% without problems.
Attached image screenshot —
The top border of the mode toolbar seems wrong now, at least when the toolbar is placed at the top of the left pane.  (See screenshot.)
Instead of changing only the top border, maybe the overall appearance of the mode toolbar could somehow be changed so that it looks better between the mail toolbar (above) and the rest of the left pane (below).
(In reply to comment #0)
> - Add a 10px space left of the Mail icon
> 
> See: http://wiki.mozilla.org/Calendar:View_Switch_Toolbar#Mode_Switch

I think when you add 10px left of the mail icon, you must add 10px on right for a symmetric UI. Or not?

Attached image 10px is missing on right —
fromm comment above
(In reply to comment #22)
The question I have is why was this 10px border added at all? Why not just add a fixed-width spacer to the toolbar left and right of the bottons as possible from the Customize Toolbar dialog? This would also allow users to remove the spacer to get a more condensed toolbar. Now a userChrome.css hack is required to remove the unnecessary 10px border.
The fixed-width spacer is wider than 10px. I would not recommend using the fixed-width spacer, because this wide space would look strange.

I don't know if we really want to remove the space. If we would do that the button would stick too the application border. This would also look strange.
(In reply to comment #23)
> Created an attachment (id=273005) [details]
> 10px is missing on right
> 
> fromm comment above
> 

Agreed, we should add this.
Just for reference, the main toolbar doesn't seem to use such a big border only 1 or 2px. See screenshots in Bug 388566 attachment 273016 [details] and attachment 272845 [details]. In "full" mode the border is added by the min-width of the icon.
Ok, let me summarize the discussion here:

1. We should reduce the 10px padding on the left side to 2px.
2. We should add a corresponding padding on the right side.
3. If possible (I'll have to talk with mickey first), we should hide the top 
   1px border, if the mode toolbar has been placed at the top of the TB sidebar

4. (This will be addressed in bug 388566)
   The icon min-width will be increased from 50px to 55px, but will only apply 
   to icon-text-mode, not icon-only- or text-only-mode

I'll add a new patch shortly.

Christian, if you can give me a short feedback on this proposal (thumbs up or down), we can probably skip the ui-review.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This fixes all items (1-4) from comment 28 and will also fix bug 388566 along the way.
Attachment #272766 - Attachment is obsolete: true
Attachment #273237 - Flags: review?(daniel.boelzle)
Comment on attachment 273237 [details] [diff] [review]
New patch with all items from comment 28 addressed

> .cal-toolbarbutton-1 {
>     -moz-box-orient: vertical;
>-    min-width: 50px;

Before your first patch the min-width for .cal-toolbarbutton-1 was set to 0px. I think thus rule should be added back. Only in fulle mode will be overwritten by min-width 55px.
New patch with the suggestion from comment 30 incorporated. Thanks Stefan!
Attachment #273237 - Attachment is obsolete: true
Attachment #273244 - Flags: review?(daniel.boelzle)
Attachment #273237 - Flags: review?(daniel.boelzle)
Attachment #273237 - Attachment description: New patch with all items from comemnt 28 addressed → New patch with all items from comment 28 addressed
Attachment #273244 - Flags: review?(daniel.boelzle) → review?(michael.buettner)
(In reply to comment #28)
> Ok, let me summarize the discussion here:
> 
> 1. We should reduce the 10px padding on the left side to 2px.
> 2. We should add a corresponding padding on the right side.
> 3. If possible (I'll have to talk with mickey first), we should hide the top 
>    1px border, if the mode toolbar has been placed at the top of the TB sidebar
> 
> 4. (This will be addressed in bug 388566)
>    The icon min-width will be increased from 50px to 55px, but will only apply 
>    to icon-text-mode, not icon-only- or text-only-mode
> 
> I'll add a new patch shortly.
> 
> Christian, if you can give me a short feedback on this proposal (thumbs up or
> down), we can probably skip the ui-review.
> 

The proposal is ok (even if I would prefere to see a 10px space ;-)
Go for it.
Christian, I think it is better to go for a uniform appearance here by following Thunderbird's lead.
Mickey, any estimate on when you might get to review this patch?
Status: REOPENED → ASSIGNED
Sorry for the delay, I'm going to get through my review queue as the first thing on Monday.
Comment on attachment 273244 [details] [diff] [review]
Patch v4 (including suggestion from comment 30)

I didn't find anything to complain about. And I'm fine with hiding the top border in case the mode toolbar is located at the top of the frame. The only issue I found is that the icons seem to be left-aligned (switch to small icons and you see it more clearly). But since this is probably out of the scope of this bug, I'd like to just go ahead with this patch. r=mickey.
Attachment #273244 - Flags: review?(michael.buettner) → review+
Keywords: checkin-needed
Attached image Screenshot —
The top half of the attached screenshot is how the mode toolbar appears at the top of the folder pane after I apply this patch.  It looks better than before the patch, but there is still some kind of border at the top of the mode toolbar (or perhaps it's at the bottom of Thunderbird's main toolbar).  There seems to be no way to completely remove it.

The following CSS is similar to what's in the patch:

#mode-toolbar
{
     border-top: none !important;
}

The bottom half of the screenshot is from after I applied the following CSS.  This is how I can make the bottom border look the same as the top border:

#mode-toolbar
{
     border-top: none !important;
     border-bottom: 2px solid rgb(150,150,150) !important;
}

I'm happy now that I'm using the second version of the CSS because I think that it looks better.  I don't think that this patch directly causes the problem and I'm not asking anyone to change the patch again.  I'm just posting this information for anyone else that wants to put the mode toolbar at the top of the folder pane instead of at the bottom.
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Verified in latest nightly build 20080108 -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: