1050403 - Search widget visual polish

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screenshot_2014-08-07-10-45-00.png

Noticing some visual inconsistencies that could use a little bit of polish just to wrap it up. 

Using this bug to track said changes. Also, attaching current state screen shot from my Nexus 5.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_widget_spec1.png

Attaching a quick spec sheet :) thanks wes!

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

I had to work some strange vudu here to keep the gray background from overlapping the orange border. Pretty close though....

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_nightly.zip

Hey wes!

Just thought I'd double check if the font color is #5F6368, at 14sp, Roboto Regular?

Also, attaching Nightly icons for sizing issues :) this is so close!

Comment 4

[:Margaret Leibovic]

Comment on attachment 8470282 [details] [diff] [review]
Patch

Review of attachment 8470282 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, I think you just might have made a copy/paste mistake in one spot.

::: mobile/android/search/res/drawable/widget_button_left.xml
@@ +5,5 @@
>  
>  <selector xmlns:android="http://schemas.android.com/apk/res/android">
>  
> +    <item android:state_pressed="true"
> +           android:drawable="@drawable/widget_button_left_active"/>

Nit: I would prefer naming the files "pressed" instead of "active", since that's the name of the state where they're being used.

@@ +10,3 @@
>  
>      <!-- The left button is gray in its off state -->
> +    <item android:drawable="@drawable/widget_button_left_inactive"/>

And then this would be wiget_button_left_default.

This would just match the other state drawables that we have in this part of the tree.

::: mobile/android/search/res/drawable/widget_button_left_active.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!-- These drawables have to be wrapped in a layer-list in order to produce padding at
> +     the bottom of the drawable. That padding ensures the drawable doesn't block the
> +     orange strip in widget_bg.9.png -->

I appreciate the comment.

@@ +12,5 @@
> +            <corners android:topLeftRadius="@dimen/widget_drawable_corner_radius"
> +                     android:topRightRadius="0dp"
> +                     android:bottomLeftRadius="0dp"
> +                     android:bottomRightRadius="0dp"/>
> +            <solid android:color="@color/widget_logo_highlight"/>

Should this be @color/widget_button_pressed?

Something is wrong here, since this file is exactly the same as widget_button_left_inactive.xml.

::: mobile/android/search/res/layout/search_widget.xml
@@ +14,5 @@
>  
>      <ImageView android:id="@+id/logo_button"
>          android:layout_width="0dp"
>          android:layout_weight="1"
> +        android:padding="7dp"

Did you get this value from antlam?

Comment 5

[Wesley Johnston (:wesj)]

(In reply to Anthony Lam (:antlam) from comment #3)
> Also, attaching Nightly icons for sizing issues :) this is so close!

If we're going to ship special icons for here, we'd need them for every variation of our logo (i.e. aurora/beta/release/nightly, optionally for developer builds). I'm not really in favor shipping extra icons just for this though.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Let's just center the icon for now then? it still seems to be riding too close to the top.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated font sizes/colors. I used Fennec colors before and forgot to update to the new ones.

(In reply to :Margaret Leibovic from comment #4)
> Nit: I would prefer naming the files "pressed" instead of "active", since
> that's the name of the state where they're being used.
> And then this would be wiget_button_left_default.
Done.

> Should this be @color/widget_button_pressed?
The logo button has a darker gray background by default. I changed this to widget_logo_default.

> Something is wrong here, since this file is exactly the same as
> widget_button_left_inactive.xml.
I think maybe the colors were wrong here or something. left and right only differ by the corner that is rounded though.
 
> Did you get this value from antlam?
Its from the mockup. i.e. 54dp tall widget - 40dp tall image = 14dp padding / 2 = 7dp per side. The logo should be centered. I wonder if some of them have a shadow underneath which shifts the image upward a bit? Maybe we will have to ship a special one for this. I just have a personal hatred of shipping lots of redundant images.

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Screenshot with mockup

Getting pretty close.

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

wooooah! looking good Wes! :)

Comment 10

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #7)

> > Did you get this value from antlam?
> Its from the mockup. i.e. 54dp tall widget - 40dp tall image = 14dp padding
> / 2 = 7dp per side. The logo should be centered. I wonder if some of them
> have a shadow underneath which shifts the image upward a bit? Maybe we will
> have to ship a special one for this. I just have a personal hatred of
> shipping lots of redundant images.

I don't think it's a redundant image if it's a different size. I think that this is fine for now, but I think we should consider adding new icons that are the correct size before shipping this for real. Our logo is pretty important, and I think it would be worth it to avoid weird scaling issues.

On desktop, there are lots of images in the branding folder, so I don't think it's that big a deal to need to create different images for each branding flavor.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch

Updated with nightly icons. I copied the xhdpi icon into the other branding folders for now as well so that we don't crash if we don't have better icons at uplift time.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8472089 [details] [diff] [review]
Patch

Review of attachment 8472089 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice.

::: mobile/android/search/res/layout/search_widget.xml
@@ +14,5 @@
>  
>      <ImageView android:id="@+id/logo_button"
>          android:layout_width="0dp"
>          android:layout_weight="1"
> +        android:padding="7dp"

This is the same value as @dimen/widget_drawable_padding. If this is not a coincidence, you could use that same dimension here (maybe update its name to something more generic, like widget_padding).

Comment 13

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/e99b11f8b3f6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e99b11f8b3f6