Closed Bug 1223898 Opened 4 years ago Closed 4 years ago

Lightning icons not shown on mail-toolbox

Categories

(SeaMonkey :: Themes, defect)

defect
Not set

Tracking

(seamonkey2.40 affected, seamonkey2.41 fixed, seamonkey2.42 fixed)

RESOLVED FIXED
seamonkey2.42
Tracking Status
seamonkey2.40 --- affected
seamonkey2.41 --- fixed
seamonkey2.42 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1211643 fixed the icons on the Lightning toolbars but not on main toolbar.
Attached patch suiteLightning.patch (obsolete) — Splinter Review
I decided to add the styles to suite instead of lightning. Through this the icon sizes can be adapted to the sizes of SM. This makes them a little bit blurry except on Classic small icons, but this looks still better than fixed 18px icons. And they are correctly aligned with the other buttons.

Philip, what do you think about this?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8686171 - Flags: review?(philip.chee)
Comment on attachment 8686171 [details] [diff] [review]
suiteLightning.patch

Mac has its own primaryToolbar.css (suite/themes/classic/mac/messenger/primaryToolbar.css) that needs to be fixed as well.
Attached patch suiteLightning.patch (obsolete) — Splinter Review
With Mac primaryToolbar.css
Attachment #8686171 - Attachment is obsolete: true
Attachment #8686171 - Flags: review?(philip.chee)
Attachment #8686198 - Flags: review?(philip.chee)
(In reply to Richard Marti (:Paenglab) from comment #1)
> I decided to add the styles to suite instead of lightning. Through this the
> icon sizes can be adapted to the sizes of SM. This makes them a little bit
> blurry except on Classic small icons, but this looks still better than fixed
> 18px icons. And they are correctly aligned with the other buttons.
Thanks for the patch.

> Philip, what do you think about this?
I'm not wildly enthused about this. Will have more comments once I eyeball the UI with this patch.
Comment on attachment 8686198 [details] [diff] [review]
suiteLightning.patch

This is sub-optimal but I guess this is the best we can do under the circumstances.

Classic and Classic-mac:

> +#lightning-button-calendar > .toolbarbutton-icon,
> +#lightning-button-tasks > .toolbarbutton-icon,
> +#extractEventButton > .box-inherit > .toolbarbutton-icon,
> +#extractTaskButton > .box-inherit > .toolbarbutton-icon {

For esoteric reasons when in the customize toolbar window you need:

#extractEventButton > .toolbarbutton-icon,
#extractTaskButton > .toolbarbutton-icon,

> +  width: 28px;
> +  height: 28px;
DOMi says the SeaMonkey toolbar images are 29x29 px

> +  width: 18px;
> +  height: 18px;
DOMi says the SeaMonkey small toolbar images are 19x19 px

Modern: no issues found.
Attachment #8686198 - Flags: review?(philip.chee) → review+
(In reply to Philip Chee from comment #5)
> Comment on attachment 8686198 [details] [diff] [review]
> suiteLightning.patch
> 
> This is sub-optimal but I guess this is the best we can do under the
> circumstances.
> 
> Classic and Classic-mac:
> 
> > +#lightning-button-calendar > .toolbarbutton-icon,
> > +#lightning-button-tasks > .toolbarbutton-icon,
> > +#extractEventButton > .box-inherit > .toolbarbutton-icon,
> > +#extractTaskButton > .box-inherit > .toolbarbutton-icon {
> 
> For esoteric reasons when in the customize toolbar window you need:
> 
> #extractEventButton > .toolbarbutton-icon,
> #extractTaskButton > .toolbarbutton-icon,

Would it be okay for you whenn I use:

#extractEventButton .toolbarbutton-icon,
#extractTaskButton .toolbarbutton-icon,

Then it would work for both cases. Or should I add both variants?
 
> > +  width: 28px;
> > +  height: 28px;
> DOMi says the SeaMonkey toolbar images are 29x29 px
> 
> > +  width: 18px;
> > +  height: 18px;
> DOMi says the SeaMonkey small toolbar images are 19x19 px

I've set it to be even to make them less blurry.
Flags: needinfo?(philip.chee)
> > For esoteric reasons when in the customize toolbar window you need:
> > 
> > #extractEventButton > .toolbarbutton-icon,
> > #extractTaskButton > .toolbarbutton-icon,
> 
> Would it be okay for you whenn I use:

> #extractEventButton .toolbarbutton-icon,
> #extractTaskButton .toolbarbutton-icon,

> Then it would work for both cases. Or should I add both variants?

CSS best practices say to favour child selectors over descendant selectors. so both variants.

> I've set it to be even to make them less blurry.
Sounds reasonable.
Flags: needinfo?(philip.chee)
Review comments applied. Carrying r+ from previous patch.

[Approval Request Comment]
Regression caused by (bug #): 1153615
User impact if declined: no lightning icon visible on main toolbar
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low, only CSS changes
String changes made by this patch: none
Attachment #8686198 - Attachment is obsolete: true
Attachment #8687405 - Flags: review+
Attachment #8687405 - Flags: approval-comm-beta?
Attachment #8687405 - Flags: approval-comm-aurora?
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/8e6a6631f908
Keywords: checkin-needed
Whiteboard: [leave open for comm-aurora and comm-beta]
Target Milestone: --- → seamonkey2.42
Comment on attachment 8687405 [details] [diff] [review]
suiteLightning.patch

Sigh. I forgot about this (somebody else normally deals with approval requests
Attachment #8687405 - Flags: approval-comm-beta?
Attachment #8687405 - Flags: approval-comm-beta+
Attachment #8687405 - Flags: approval-comm-aurora?
Attachment #8687405 - Flags: approval-comm-aurora+
Comment on attachment 8687405 [details] [diff] [review]
suiteLightning.patch

And of course at this moment 2.40 is comm-release 2.41 is comm-beta
Attachment #8687405 - Flags: approval-comm-aurora+ → approval-comm-release+
Pushed to comm-beta (SeaMonkey 2.41)
http://hg.mozilla.org/releases/comm-beta/rev/cf1b57ad7dd6
Pushed to comm-release (SeaMonkey 2.40)
http://hg.mozilla.org/releases/comm-release/rev/0ea850b43925
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Blocks: SM2.40
Whiteboard: [leave open for comm-aurora and comm-beta]
Blocks: 1236982
You need to log in before you can comment on or make changes to this bug.