Closed
Bug 493022
Opened 14 years ago
Closed 14 years ago
Add mozapps/ to the modern theme
Categories
(SeaMonkey :: Themes, enhancement)
SeaMonkey
Themes
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0b2
People
(Reporter: prometeo.bugs, Assigned: prometeo.bugs)
References
Details
(Keywords: modern)
Attachments
(3 files, 3 obsolete files)
9.41 KB,
application/x-jar
|
Details | |
765 bytes,
image/png
|
Details | |
78.23 KB,
patch
|
philip.chee
:
review+
philip.chee
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; it; rv:1.9.0.10) Gecko/2009042700 SUSE/3.0.10-1.1.1 Firefox/3.0.10 Build Identifier: Using the work done by Kuden, update the modern theme with the mozapps dir. Reproducible: Always
Assignee | ||
Comment 1•14 years ago
|
||
First version of the patch: * Added 2 icons to global/icons/because they are referenced by some css * renamed downloadButtons.png -> buttons.png (and relative css files) to match *stripe * left downloads in, even if it's not used yet * added boilerplate to all css files, citing Kuden
Attachment #377449 -
Flags: review?(neil)
Assignee: nobody → prometeo.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 2•14 years ago
|
||
Comment on attachment 377449 [details] [diff] [review] The whole package >diff --git a/suite/themes/modern/global/icons/Error.png b/suite/themes/modern/global/icons/Error.png I guess alert-error.gif is the wrong size for this. >diff --git a/suite/themes/modern/global/icons/loading_16.png b/suite/themes/modern/global/icons/loading_16.png chrome://communicator/skin/icons/loading.gif
Comment 3•14 years ago
|
||
Comment on attachment 377449 [details] [diff] [review] The whole package >+ skin/modern/mozapps/downloads/buttons.png (mozapps/downloads/buttons.png) >+ skin/modern/mozapps/downloads/downloadIcon.png (mozapps/downloads/downloadIcon.png) >+ skin/modern/mozapps/downloads/downloads.css (mozapps/downloads/downloads.css) >+ skin/modern/mozapps/downloads/unknownContentType.css (mozapps/downloads/unknownContentType.css) downloads will be hard to test, but not impossible... >+ skin/modern/mozapps/passwordmgr/key.png (mozapps/passwordmgr/key.png) We don't use the mozapps passwordmgr... >+ skin/modern/mozapps/places/defaultFavicon.png (mozapps/places/defaultFavicon.png) >+ skin/modern/mozapps/places/tagContainerIcon.png (mozapps/places/tagContainerIcon.png) Or the mozapps places... >+ skin/modern/mozapps/profile/profileicon.png (mozapps/profile/profileicon.png) >+ skin/modern/mozapps/profile/profileSelection.css (mozapps/profile/profileSelection.css) Or the mozapps profile selection...
Comment 4•14 years ago
|
||
Comment on attachment 377449 [details] [diff] [review] The whole package I've been looking at the icons. I noticed a) update.png is a copy of the left half of extensionIcons.png b) you have two copies of buttons.png The image editor that creates those images strangely uses white background for areas with 100% alpha transparency except for the outer border which is black...
Comment 5•14 years ago
|
||
Comment on attachment 377449 [details] [diff] [review] The whole package As well as the comments previously mentioned, these changes seem to depend on some changes to global/richlistbox.css yet to be posted. >+ border-top-width: 0; >+ border-right-width: 0; >+ border-bottom: 2px solid; >+ border-left-width: 0; >+ -moz-border-bottom-colors: #E0ECF6 #C7D0D9; Instead of border-XXX-width: 0; I prefer border-XXX: none; Assuming the richlistbox CSS changes are what I think they are, you won't need the bottom colours, but I can't tell without seeing them, not that it matters, since nobody will be seeing this file anyway. >+richlistitem[type="download"][selected="true"] { >+ background-image: url("chrome://mozapps/skin/extensions/itemEnabledFader.png"); Download manager referring to a file in extensions? Any shared images belong in a shared location, e.g. mozapps/icons >+ border: 1px solid; >+ -moz-border-top-colors: rgb(110, 115, 120); >+ -moz-border-right-colors: rgb(238, 240, 243); >+ -moz-border-bottom-colors: rgb(238, 240, 243); >+ -moz-border-left-colors: rgb(110, 115, 120); Single colour borders should simply be specified using border-XXX: 1px solid #XXXXXX; I saw the rgb() used somewhere else by mistake too. >+#extensionsBox { >+ margin: 10px 10px 0px 10px; I thought this a little large: try 7px 5px 0px; >+#resizerBox { >+ margin-top: -12px; >+ visibility: hidden; Should be -15px to match the resizer (we also don't need either visibility.) >+#commandBarBottom { >+ margin: 10px 10px 5px 10px; Try 7px 5px 5px; >+ padding-top: 6px; >+ padding-bottom: 6px; >+ -moz-padding-start: 7px; >+ -moz-padding-end: 7px; padding: 6px 7px; >+.updateBadge { >+ -moz-margin-end: -2px; >+ -moz-image-region: rect(0px 16px 16px 0px); >+} >+ >+.notifyBadge { >+ -moz-margin-start: -2px; Put this margin in the shared style rule instead. >+ -moz-image-region: rect(0px 48px 16px 32px); Why such a gap between the images? >+/* Selected Add-on buttons >+ See content/extensions.css to hide / display buttons */ I don't see the point of this margin tweaking. Just remove it please. >+ list-style-image: url("chrome://global/skin/icons/loading_16.png"); I might have mentioned that this is wrong. There are places where the wrong throbber is used too.
Attachment #377449 -
Flags: review?(neil) → review-
![]() |
||
Comment 7•14 years ago
|
||
First the unfixed nits: > I've been looking at the icons. I noticed a) update.png is a copy of the left > half of extensionIcons.png Not sure what to do here. > The image editor that creates those images strangely uses white background for > areas with 100% alpha transparency except for the outer border which is > black... Sorry. I don't know how to fix this. > >+.updateBadge { > >+ -moz-margin-end: -2px; > >+ -moz-image-region: rect(0px 16px 16px 0px); > >+} > >+ > >+.notifyBadge { > >+ -moz-margin-start: -2px; > Put this margin in the shared style rule instead. I don't understand why? ################################################################## > >diff --git a/suite/themes/modern/global/icons/Error.png b/suite/themes/modern/global/icons/Error.png > I guess alert-error.gif is the wrong size for this. Also alert-error.gif doesn't have a transparent background. > >diff --git a/suite/themes/modern/global/icons/loading_16.png b/suite/themes/modern/global/icons/loading_16.png > chrome://communicator/skin/icons/loading.gif Removed and Fixed all references to loading_16.png > downloads will be hard to test, but not impossible... Setting browser.download.manager.useToolkitUI to true seems to work. > We don't use the mozapps passwordmgr... Fixed. > Or the mozapps places... Fixed. > Or the mozapps profile selection... Fixed. > I've been looking at the icons. I noticed a) update.png is a copy of the left > half of extensionIcons.png Not sure what to do here. > b) you have two copies of buttons.png Fixed. Moved to mozapps/icons/ > The image editor that creates those images strangely uses white background for > areas with 100% alpha transparency except for the outer border which is > black... Sorry. I don't know how to fix this. > (From update of attachment 377449 [details] [diff] [review]) > As well as the comments previously mentioned, these changes seem to depend on > some changes to global/richlistbox.css yet to be posted. Richlistbox.css updated in the modern->global bug. > >+ border-top-width: 0; > >+ border-right-width: 0; > >+ border-bottom: 2px solid; > >+ border-left-width: 0; > >+ -moz-border-bottom-colors: #E0ECF6 #C7D0D9; > Instead of border-XXX-width: 0; I prefer border-XXX: none; Fixed. > Assuming the richlistbox CSS changes are what I think they are, you won't need > the bottom colours, but I can't tell without seeing them, not that it matters, Fixed. > since nobody will be seeing this file anyway. ?? > >+richlistitem[type="download"][selected="true"] { > >+ background-image: url("chrome://mozapps/skin/extensions/itemEnabledFader.png"); > Download manager referring to a file in extensions? Any shared images belong in > a shared location, e.g. mozapps/icons Fixed. > >+ border: 1px solid; > >+ -moz-border-top-colors: rgb(110, 115, 120); > >+ -moz-border-right-colors: rgb(238, 240, 243); > >+ -moz-border-bottom-colors: rgb(238, 240, 243); > >+ -moz-border-left-colors: rgb(110, 115, 120); > Single colour borders should simply be specified using > border-XXX: 1px solid #XXXXXX; Fixed. > I saw the rgb() used somewhere else by mistake too. Fixed globally. > >+#extensionsBox { > >+ margin: 10px 10px 0px 10px; > I thought this a little large: try 7px 5px 0px; Fixed. > >+#resizerBox { > >+ margin-top: -12px; > >+ visibility: hidden; > Should be -15px to match the resizer (we also don't need either visibility.) Fixed. > >+#commandBarBottom { > >+ margin: 10px 10px 5px 10px; > Try 7px 5px 5px; Fixed. > >+ padding-top: 6px; > >+ padding-bottom: 6px; > >+ -moz-padding-start: 7px; > >+ -moz-padding-end: 7px; > padding: 6px 7px; Fixed. > >+.updateBadge { > >+ -moz-margin-end: -2px; > >+ -moz-image-region: rect(0px 16px 16px 0px); > >+} > >+ > >+.notifyBadge { > >+ -moz-margin-start: -2px; > Put this margin in the shared style rule instead. Why? > >+ -moz-image-region: rect(0px 48px 16px 32px); > Why such a gap between the images? It appears to have been copied from winstripe. The pinstripe version as four icons. Perhaps the original winstripe image also had four? > >+/* Selected Add-on buttons > >+ See content/extensions.css to hide / display buttons */ > I don't see the point of this margin tweaking. Just remove it please. Fixed. > >+ list-style-image: url("chrome://global/skin/icons/loading_16.png"); > I might have mentioned that this is wrong. There are places where the wrong > throbber is used too. Fixed globally.
Attachment #377449 -
Attachment is obsolete: true
Attachment #395770 -
Flags: review?(neil)
Comment 8•14 years ago
|
||
(In reply to comment #7) > > >+.updateBadge { > > >+ -moz-margin-end: -2px; > > >+ -moz-image-region: rect(0px 16px 16px 0px); > > >+} > > >+ > > >+.notifyBadge { > > >+ -moz-margin-start: -2px; > > Put this margin in the shared style rule instead. > I don't understand why? Why write the same line twice when you only have to do it once?
Comment 9•14 years ago
|
||
(In reply to comment #7) > > I've been looking at the icons. I noticed a) update.png is a copy of the left > > half of extensionIcons.png > Not sure what to do here. Bah, I should never have looked at this. xpinstall/xpinstallItemGeneric.png and extensions/themeGeneric.png are 32x32 icons; extensions/extensionIcons.png has the same two icons at 16x16, and update.png has a copy too, and at least three of these have to be separate anyway...
![]() |
||
Comment 10•14 years ago
|
||
So what would you suggest? Making them all individual icons and putting them in mozapps/icons/?
Comment 11•14 years ago
|
||
Notes: notifyBadges.png split up into notifyBadge.png and updateBadge.png ratings.png simplified, will need new image regions buttons.png should be removed and suite download manager icons used instead update.png and item*Fader.png were already optimised so not included here plugin* are all copies so this zip only includes one at each size
Comment 12•14 years ago
|
||
+.addonRating[rating] { + display: -moz-box; + width: 60px; + height: 12px; + list-style-image: url("chrome://mozapps/skin/extensions/ratings.png"); +} + +.addonRating[rating="0"] { + -moz-image-region: rect(0px 60px 12px 0px); +} + +.addonRating[rating="1"] { + -moz-image-region: rect(0px 72px 12px 12px); +} + +.addonRating[rating="2"] { + -moz-image-region: rect(0px 84px 12px 24px); +} + +.addonRating[rating="3"] { + -moz-image-region: rect(0px 96px 12px 36px); +} + +.addonRating[rating="4"] { + -moz-image-region: rect(0px 108px 12px 48px); +} + +.addonRating[rating="5"] { + -moz-image-region: rect(0px 120px 12px 60px); +}
Comment 13•14 years ago
|
||
(In reply to comment #10) > So what would you suggest? Making them all individual icons and putting them in > mozapps/icons/? Sadly we can't put them in mozapps/icons/ because their locations are hardcoded in various source files :-(
Comment 14•14 years ago
|
||
Comment on attachment 395770 [details] [diff] [review] Patch v1.0 >+ border-bottom: 2px solid; Don't need this, it's already 2px solid. >+ border: 1px solid !important; Make this 1px dotted here since it's transparent anyway. >+.mini-button:focus { >+ border-style: dotted !important; Thus avoiding having to change it here. >+#clearListButton { >+ margin-top: 5px; >+ margin-bottom: 5px; >+ -moz-margin-start: 5px; >+} Looks fine to me without these margins but you could try margin: 2px 4px; or margin: 4px; >+.small-indent { >+ -moz-margin-start: 15px; >+ -moz-margin-end: 15px; Use margin-left and margin-right when the values are the same. >+#extensionDescription { >+ color: #324055; Strange choice of colour... >+ list-style-image: url("chrome://communicator/skin/icons/loading.gif"); >+ -moz-image-region: auto; I guess we won't need that now that my "new" badge image needs no region. >+.throbber { >+ list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif"); Oops >+ margin-top: 1px; >+ margin-bottom: 4px; >+ -moz-margin-start: 5px; >+ -moz-margin-end: 5px; margin: 1px 5px 4px; >+#moreDetails { >+ margin: 1px 5px 4px 3px; Oops, not RTL-safe ;-) >+ margin: 1px 5px 4px 5px; Can write 1px 5px 4px; >+#downloadThrobber { >+ margin-left: 5px; -moz-margin-start? >+#downloadStatusProgress { >+ padding-right: 5px; -moz-padding-start? >+#verificationFailedIcon { >+ margin-left: 5px; margin-start? >+ margin: 1px 5px 4px 5px; 1px 5px 4px; >+ margin-left: 27px; margin-start?
![]() |
||
Comment 15•14 years ago
|
||
> Notes: > notifyBadges.png split up into notifyBadge.png and updateBadge.png Noted. CSS adjusted. > ratings.png simplified, will need new image regions Fixed. > buttons.png should be removed and suite download manager icons used instead Fixed. > update.png and item*Fader.png were already optimised so not included here > plugin* are all copies so this zip only includes one at each size OK. > +.addonRating[rating] { > +.addonRating[rating="0"] { > +.addonRating[rating="1"] { > +.addonRating[rating="2"] { > +.addonRating[rating="3"] { > +.addonRating[rating="4"] { > +.addonRating[rating="5"] { Fixed. > >+ border-bottom: 2px solid; > Don't need this, it's already 2px solid. Fixed. > >+ border: 1px solid !important; > Make this 1px dotted here since it's transparent anyway. Fixed. > >+.mini-button:focus { > >+ border-style: dotted !important; > Thus avoiding having to change it here. Fixed. > >+#clearListButton { > >+ margin-top: 5px; > >+ margin-bottom: 5px; > >+ -moz-margin-start: 5px; > >+} > Looks fine to me without these margins but you could try > margin: 2px 4px; or margin: 4px; Fixed using 2px 4px. > >+.small-indent { > >+ -moz-margin-start: 15px; > >+ -moz-margin-end: 15px; > Use margin-left and margin-right when the values are the same. Fixed. > >+#extensionDescription { > >+ color: #324055; > Strange choice of colour... Any suggestions? They are all just funny hex numbers to me. > >+ list-style-image: url("chrome://communicator/skin/icons/loading.gif"); > >+ -moz-image-region: auto; > I guess we won't need that now that my "new" badge image needs no region. Fixed. > >+.throbber { > >+ list-style-image: url("chrome://global/skin/throbber/Throbber-small.gif"); > Oops Fixed. > >+ margin-top: 1px; > >+ margin-bottom: 4px; > >+ -moz-margin-start: 5px; > >+ -moz-margin-end: 5px; > margin: 1px 5px 4px; Fixed. > >+#moreDetails { > >+ margin: 1px 5px 4px 3px; > Oops, not RTL-safe ;-) Fixed. > >+ margin: 1px 5px 4px 5px; > Can write 1px 5px 4px; Fixed. > >+#downloadThrobber { > >+ margin-left: 5px; > -moz-margin-start? Fixed. > >+#downloadStatusProgress { > >+ padding-right: 5px; > -moz-padding-start? I used -moz-padding-end. > >+#verificationFailedIcon { > >+ margin-left: 5px; > margin-start? Fixed. > >+ margin: 1px 5px 4px 5px; > 1px 5px 4px; Fixed. > >+ margin-left: 27px; > margin-start? Fixed.
Attachment #395770 -
Attachment is obsolete: true
Attachment #396065 -
Flags: superreview?(neil)
Attachment #396065 -
Flags: review?(neil)
Attachment #395770 -
Flags: review?(neil)
Comment 16•14 years ago
|
||
about.css uses #22262F so I guess that's as good as any ;-)
Comment 17•14 years ago
|
||
350: trailing whitespace. margin-top: 1px; 365: trailing whitespace. margin-left: 15px; 370: trailing whitespace. -moz-margin-start: 0; 1238: trailing whitespace. list-style-image: url("chrome://mozapps/skin/update/update.png"); 1327: trailing whitespace.
Comment 18•14 years ago
|
||
Comment on attachment 395888 [details]
Optimised icons
Whoops, I accidentally switched notifyBadge and updateBadge :-(
Comment 19•14 years ago
|
||
I got this back-to-front, so that the CSS I gave for a rating of 4 is really a rating of 1 with this corrected image etc.
Comment 20•14 years ago
|
||
Comment on attachment 396065 [details] [diff] [review] Patch v1.1 >+/* Not Implemented Yet. >+.pause[disabled="true"] { >+ -moz-image-region: rect(); >+} Actually I think an opacity of 0.5 might work.
Attachment #396065 -
Flags: superreview?(neil)
Attachment #396065 -
Flags: superreview+
Attachment #396065 -
Flags: review?(neil)
Attachment #396065 -
Flags: review+
![]() |
||
Updated•14 years ago
|
Attachment #396196 -
Attachment is patch: false
![]() |
||
Updated•14 years ago
|
Attachment #396196 -
Attachment mime type: text/plain → image/png
![]() |
||
Comment 21•14 years ago
|
||
Carrying forward r+/sr+ from Neil > about.css uses #22262F so I guess that's as good as any ;-) Fixed. > 350: trailing whitespace. > 365: trailing whitespace. > 370: trailing whitespace. > 1238: trailing whitespace. > 1327: trailing whitespace. Fixed. > Whoops, I accidentally switched notifyBadge and updateBadge :-( Fixed. > Created an attachment (id=396196) [details] > Fixed ratings.png > > I got this back-to-front, so that the CSS I gave for a rating of 4 is really a > rating of 1 with this corrected image etc. Fixed. >>+/* Not Implemented Yet. >>+.pause[disabled="true"] { >>+ -moz-image-region: rect(); >>+} > Actually I think an opacity of 0.5 might work. Fixed.
Attachment #396065 -
Attachment is obsolete: true
Attachment #396206 -
Flags: superreview+
Attachment #396206 -
Flags: review+
![]() |
||
Updated•14 years ago
|
Attachment #396206 -
Attachment description: Patch v1.2 Fixed remaining problems. r=neil sr=neil → [for checkin] Patch v1.2 Fixed remaining problems. r=neil sr=neil
![]() |
||
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [patch for checkin comment 21]
Comment 22•14 years ago
|
||
Comment on attachment 396206 [details] [diff] [review] Patch v1.2 Fixed remaining problems [Checkin: Comment 22] http://hg.mozilla.org/comm-central/rev/7a15328eaa67
Attachment #396206 -
Attachment description: [for checkin] Patch v1.2 Fixed remaining problems. r=neil sr=neil → Patch v1.2 Fixed remaining problems
[Checkin: Comment 22]
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed → modern
Resolution: --- → FIXED
Whiteboard: [patch for checkin comment 21]
Target Milestone: --- → seamonkey2.0b2
Comment 23•14 years ago
|
||
Could it be that this checkin removed the icons from add-on manager in modern theme accidentally?
![]() |
||
Comment 24•14 years ago
|
||
> Could it be that this checkin removed the icons from add-on manager in modern
> theme accidentally?
No. It wasn't accidental.
Comment 25•14 years ago
|
||
(In reply to comment #24) > No. It wasn't accidental. So for what reason were they removed? At the moment without the icons the add-on manager doesn't look very nice.
You need to log in
before you can comment on or make changes to this bug.
Description
•