Closed
Bug 1162584
Opened 9 years ago
Closed 9 years ago
Update install flow with new icons from bug 1144599
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
People
(Reporter: mossop, Assigned: mossop)
References
Details
(Whiteboard: [fxsearch][hijacking])
Attachments
(1 file)
(deleted),
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
Once we have the new artwork we need to sweep through and update them in the tree.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dtownsend
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [fxsearch][hijacking]
Assignee | ||
Comment 1•9 years ago
|
||
This adds the new icons and updates the styles. For the install complete notification I've split it into two doorhangers to make styling the icon easier. For the confirmation doorhanger it's a little harder since it is a more complex doorhanger so I'm just adding a warning class where we need to show the warning icon.
Attachment #8606010 -
Flags: review?(dao)
Comment 2•9 years ago
|
||
Comment on attachment 8606010 [details] [diff] [review]
patch
>+ let notification = document.getElementById("addon-install-confirmation-notification");
> if (unsigned.length == installInfo.installs.length) {
> // None of the add-ons are verified
> messageString = gNavigatorBundle.getString("addonConfirmInstallUnsigned.message");
>+ notification.classList.add("warning");
> }
> else if (unsigned.length == 0) {
> // All add-ons are verified or don't need to be verified
> messageString = gNavigatorBundle.getString("addonConfirmInstall.message");
>+ notification.classList.remove("warning");
> }
> else {
> // Some of the add-ons are unverified, the list of names will indicate
> // which
> messageString = gNavigatorBundle.getString("addonConfirmInstallSomeUnsigned.message");
>+ notification.classList.add("warning");
> }
>+#addon-install-confirmation-notification.warning .popup-notification-icon[popupid="addon-install-confirmation"] {
>+ list-style-image: url(chrome://browser/skin/addons/addons-warning.svg);
>+}
In our code base, classes are supposed to be unambiguous such that you don't get unintentionally affected by some other styles using that class. So please name this class distinctively or use an attribute (e.g. #addon-install-confirmation-notification[warning]).
>+ skin/classic/browser/addons/addons-blocked.svg (../shared/addons/addons-blocked.svg)
>+ skin/classic/browser/addons/addons-confirm.svg (../shared/addons/addons-confirm.svg)
>+ skin/classic/browser/addons/addons-downloading.svg (../shared/addons/addons-downloading.svg)
>+ skin/classic/browser/addons/addons-error.svg (../shared/addons/addons-error.svg)
>+ skin/classic/browser/addons/addons-installed.svg (../shared/addons/addons-installed.svg)
>+ skin/classic/browser/addons/addons-restart.svg (../shared/addons/addons-restart.svg)
>+ skin/classic/browser/addons/addons-warning.svg (../shared/addons/addons-warning.svg)
addon-install-blocked.svg, addon-install-confirm.svg, addon-install-downloading.svg etc.
>+ skin/classic/browser/addons/notifications-addons.svg (../shared/addons/notifications-addons.svg)
addon-install-anchor.svg
>+#addons-notification-icon:active {
>+ list-style-image: url(chrome://browser/skin/addons/notifications-addons.svg#active);
> }
should be :hover:active rather than :active
Attachment #8606010 -
Flags: review?(dao) → review+
Comment 3•9 years ago
|
||
Comment on attachment 8606010 [details] [diff] [review]
patch
Review of attachment 8606010 [details] [diff] [review]:
-----------------------------------------------------------------
A couple of comments about the SVGs. The comments on addons-blocked.svg apply to all SVG files.
::: browser/themes/shared/addons/addons-blocked.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
nit : This DOCTYPE can be removed.
@@ +6,5 @@
> + xmlns:xlink="http://www.w3.org/1999/xlink"
> + width="64"
> + height="64"
> + viewBox="0 0 64 64">
> +
nit : Lots of trailing whitespace here and throughout the SVG files.
@@ +10,5 @@
> +
> + <defs>
> +
> + <style type="text/css">
> + <![CDATA[
nit : useless wrapping with CDATA
@@ +14,5 @@
> + <![CDATA[
> + .style-puzzle-piece {
> + fill: url('#gradient-linear-puzzle-piece');
> + }
> +
nit : trailing whitespace
@@ +16,5 @@
> + fill: url('#gradient-linear-puzzle-piece');
> + }
> +
> + .style-badge-shadow {
> + fill: #0d131a;
nit : trailing whitespace
@@ +21,5 @@
> + fill-opacity: .15;
> + }
> +
> + .style-badge-background {
> + fill: #fff;
nit : trailing whitespace
@@ +23,5 @@
> +
> + .style-badge-background {
> + fill: #fff;
> + }
> +
nit : trailing whitespace
@@ +25,5 @@
> + fill: #fff;
> + }
> +
> + .style-badge-inside {
> + fill: #e62117;
nit : trailing whitespace
@@ +27,5 @@
> +
> + .style-badge-inside {
> + fill: #e62117;
> + }
> +
nit : trailing whitespace
@@ +29,5 @@
> + fill: #e62117;
> + }
> +
> + .style-badge-icon {
> + fill: #fff;
nit : trailing whitespace
@@ +31,5 @@
> +
> + .style-badge-icon {
> + fill: #fff;
> + }
> +
nit : trailing whitespace
@@ +49,5 @@
> + <circle class="style-badge-background" r="15" cy="15" cx="16" />
> + <circle class="style-badge-inside" r="12" cy="15" cx="16" />
> + <rect class="style-badge-icon" x="9" y="13" width="14" height="4" rx="1" ry="1" />
> + </svg>
> +
nit : trailing whitespace
::: browser/themes/shared/addons/addons-confirm.svg
@@ +6,5 @@
> + xmlns:xlink="http://www.w3.org/1999/xlink"
> + width="64"
> + height="64"
> + viewBox="0 0 64 64">
> +
nit : trailing whitespace
@@ +10,5 @@
> +
> + <defs>
> +
> + <style type="text/css">
> + <![CDATA[
nit : useless wrapping with CDATA
@@ +14,5 @@
> + <![CDATA[
> + .style-puzzle-piece {
> + fill: url('#gradient-linear-puzzle-piece');
> + }
> +
nit : trailing whitespace
@@ +26,5 @@
> +
> + </defs>
> +
> + <path id="puzzle-piece" class="style-puzzle-piece" d="M42,62c2.2,0,4-1.8,4-4l0-14.2c0,0,0.4-3.7,2.8-3.7c2.4,0,2.2,3.9,6.7,3.9c2.3,0,6.2-1.2,6.2-8.2 c0-7-3.9-7.9-6.2-7.9c-4.5,0-4.3,3.7-6.7,3.7c-2.4,0-2.8-3.8-2.8-3.8V22c0-2.2-1.8-4-4-4H31.5c0,0-3.4-0.6-3.4-3 c0-2.4,3.8-2.6,3.8-7.1c0-2.3-1.3-5.9-8.3-5.9s-8,3.6-8,5.9c0,4.5,3.4,4.7,3.4,7.1c0,2.4-3.4,3-3.4,3H6c-2.2,0-4,1.8-4,4l0,7.8 c0,0-0.4,6,4.4,6c3.1,0,3.2-4.1,7.3-4.1c2,0,4,1.9,4,6c0,4.2-2,6.3-4,6.3c-4,0-4.2-4.1-7.3-4.1c-4.8,0-4.4,5.8-4.4,5.8L2,58 c0,2.2,1.8,4,4,4H19c0,0,6.3,0.4,6.3-4.4c0-3.1-4-3.6-4-7.7c0-2,2.2-4.5,6.4-4.5c4.2,0,6.6,2.5,6.6,4.5c0,4-3.9,4.6-3.9,7.7 c0,4.9,6.3,4.4,6.3,4.4H42z"/>
> +
nit : trailing whitespace
Comment 5•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•9 years ago
|
Iteration: --- → 41.1 - May 25
Assignee | ||
Comment 6•9 years ago
|
||
Decided we won't uplift this to 40.
Updated•9 years ago
|
QA Contact: vasilica.mihasca
Comment 7•9 years ago
|
||
Verified fixed on Firefox 41.0a1 (2015-06-08) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5.
The testing was based on http://people.mozilla.org/~shorlander/mockups/Add-ons-Install-Flow/Add-ons-Install-Flow-Icons.html
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•