Closed Bug 1162584 Opened 8 years ago Closed 8 years ago

Update install flow with new icons from bug 1144599

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- wontfix
firefox41 --- verified

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [fxsearch][hijacking])

Attachments

(1 file)

Once we have the new artwork we need to sweep through and update them in the tree.
Depends on: 1144599
No longer depends on: 1120996
Summary: Update install flow with new icons from bug 1120996 → Update install flow with new icons from bug 1144599
Assignee: nobody → dtownsend
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [fxsearch][hijacking]
Attached patch patchSplinter Review
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 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 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
https://hg.mozilla.org/mozilla-central/rev/a412f9c18d6b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1165609
Depends on: 1165610
Iteration: --- → 41.1 - May 25
Depends on: 1167198
No longer depends on: 1167198
Decided we won't uplift this to 40.
QA Contact: vasilica.mihasca
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.