Search widget visual polish

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: antlam, Assigned: wesj)

Tracking

unspecified
Firefox 34
x86
Android
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Posted image Screenshot_2014-08-07-10-45-00.png (obsolete) —
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.
Blocks: widget
Posted image prev_widget_spec1.png (obsolete) —
Attaching a quick spec sheet :) thanks wes!
Flags: needinfo?(wjohnston)
Posted patch Patch (obsolete) — Splinter Review
I had to work some strange vudu here to keep the gray background from overlapping the orange border. Pretty close though....
Attachment #8470282 - Flags: review?(margaret.leibovic)
Flags: needinfo?(wjohnston)
Posted file icon_nightly.zip (obsolete) —
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!
Flags: needinfo?(wjohnston)
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?
Attachment #8470282 - Flags: review?(margaret.leibovic) → feedback+
(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.
Flags: needinfo?(wjohnston)
Let's just center the icon for now then? it still seems to be riding too close to the top.
Flags: needinfo?(wjohnston)
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8470282 - Attachment is obsolete: true
Attachment #8471188 - Flags: review?(margaret.leibovic)
Flags: needinfo?(wjohnston)
Getting pretty close.
wooooah! looking good Wes! :)
(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.
Assignee: nobody → wjohnston
Posted patch PatchSplinter Review
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.
Attachment #8469422 - Attachment is obsolete: true
Attachment #8469451 - Attachment is obsolete: true
Attachment #8470898 - Attachment is obsolete: true
Attachment #8471188 - Attachment is obsolete: true
Attachment #8471188 - Flags: review?(margaret.leibovic)
Attachment #8472089 - Flags: review?(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).
Attachment #8472089 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/e99b11f8b3f6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.