Closed Bug 1058660 Opened 5 years ago Closed 5 years ago

Tweak history panel header color for better contrast with the tab strip background

Categories

(Firefox for Android :: Theme and Visual Design, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 --- verified
firefox35 --- verified
firefox36 --- verified

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(9 files)

We changed the tab strip background color recently.
No longer blocks: new-toolbar-v1
Attaching current state screen shot
I've got 2 options here that I could use some feedback on.. tell me what you think :)

I also wanted to ask what size and color the text for these dividers are "Last Week", "Yesterday", etc...
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Anthony Lam (:antlam) from comment #2)
> Created attachment 8479171 [details]
> prev_dividermatte_mock1.png
> 
> I've got 2 options here that I could use some feedback on.. tell me what you
> think :)
> 
> I also wanted to ask what size and color the text for these dividers are
> "Last Week", "Yesterday", etc...

I kinda prefer the visual simplicity of option 1. Looks a little less noisy. Is it the same color than the title strip on top?
Flags: needinfo?(lucasr.at.mozilla)
Yes! It's completely dependent on the divider color to separate the two. 

The matte will be #F5F5F5 (matching the panel's header background) while the divider is #D7D9DB.

We might also want to change the text color to #777777 to match the header color. :)

#D7D9DB is also the color of the shadow that I have in my design but I just realized we haven't really explicitly talked about that. Is that magically the color we're using right now for the shadow too? :P
Flags: needinfo?(lucasr.at.mozilla)
Is the divider below the title strip always there, on all panels? Or does it only apply to the history panel?
Flags: needinfo?(lucasr.at.mozilla) → needinfo?(alam)
I just realized that these dividers are probably part of the element usually above it and so the first element would not have a divider above. Since it's the same color as the Panel label background, they would clash when displayed next to each other.

Could we just apply the divider specifically to situations in panels where the first item is one of these "sub headers"? Like History and Recent Tabs..

Thoughts?
Flags: needinfo?(alam)
this was really bothering me lately hah ^ :)
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 8503191 [details] [diff] [review]
Draw divider at the bottom of about:home's tab strip (r=margaret)

I realize that this is not what antlam proposes in comment #6 but I'm confident that this is what he actually meant ;-)
Attachment #8503191 - Flags: review?(margaret.leibovic)
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 8503192 [details] [diff] [review]
Use consistent height in about:home's tab strip (r=margaret)

We're using 40dp on phones, it makes sense to use the same on tabets. Bigger tap areas and all.
Attachment #8503192 - Flags: review?(margaret.leibovic)
Comment on attachment 8503193 [details] [diff] [review]
Use consistent bg color in about:home's tab strip (r=margaret)

Same color than the toolbar.
Attachment #8503193 - Flags: review?(margaret.leibovic)
Comment on attachment 8503194 [details] [diff] [review]
Don't use top divider on about:home lists (r=margaret)

Now that we draw the divider at the bottom of about:home's tabs strip, there's no need to use the top divider in the list views.
Attachment #8503194 - Flags: review?(margaret.leibovic)
Attached image Screenshot
This is how it looks.
Attachment #8503191 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8503192 [details] [diff] [review]
Use consistent height in about:home's tab strip (r=margaret)

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

::: mobile/android/base/resources/layout-large-v11/home_pager.xml
@@ +13,5 @@
>                                    android:layout_height="match_parent"
>                                    android:background="@android:color/white">
>  
>      <org.mozilla.gecko.home.TabMenuStrip android:layout_width="match_parent"
> +                                         android:layout_height="40dip"

Can you pull this value out into a dimen so that it's more obvious that it's consistent with where it's used for HomePagerTabStrip?

Also, the naming of HomePagerTabStrip and TabMenuStrip always confuses me! Maybe we can rename those at some point :)
Attachment #8503192 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8503193 [details] [diff] [review]
Use consistent bg color in about:home's tab strip (r=margaret)

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

Nice. Consistency, one style at a time! :)
Attachment #8503193 - Flags: review?(margaret.leibovic) → review+
Attachment #8503194 - Flags: review?(margaret.leibovic) → review+
If these patches cause no problems on Nightly, I think they would be a nice win to uplift all the way to beta to be part of the new toolbar UI release.
(In reply to :Margaret Leibovic from comment #17)
> Comment on attachment 8503192 [details] [diff] [review]
> Use consistent height in about:home's tab strip (r=margaret)
> 
> Review of attachment 8503192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout-large-v11/home_pager.xml
> @@ +13,5 @@
> >                                    android:layout_height="match_parent"
> >                                    android:background="@android:color/white">
> >  
> >      <org.mozilla.gecko.home.TabMenuStrip android:layout_width="match_parent"
> > +                                         android:layout_height="40dip"
> 
> Can you pull this value out into a dimen so that it's more obvious that it's
> consistent with where it's used for HomePagerTabStrip?

Good idea, done.

> Also, the naming of HomePagerTabStrip and TabMenuStrip always confuses me!
> Maybe we can rename those at some point :)

AGREE ;-)
Comment on attachment 8503191 [details] [diff] [review]
Draw divider at the bottom of about:home's tab strip (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503191 - Flags: approval-mozilla-beta?
Attachment #8503191 - Flags: approval-mozilla-aurora?
Comment on attachment 8503192 [details] [diff] [review]
Use consistent height in about:home's tab strip (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503192 - Flags: approval-mozilla-beta?
Attachment #8503192 - Flags: approval-mozilla-aurora?
Comment on attachment 8503193 [details] [diff] [review]
Use consistent bg color in about:home's tab strip (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503193 - Flags: approval-mozilla-beta?
Attachment #8503193 - Flags: approval-mozilla-aurora?
Comment on attachment 8503194 [details] [diff] [review]
Don't use top divider on about:home lists (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1010740
[User impact if declined]: Unpolished tab strip in about:home. Make us look amateur.
[Describe test coverage new/current, TBPL]: Local tests only. Let's bake this in Nightly for a couple days and then uplift.
[Risks and why]: Very low. Only visual tweaks.
[String/UUID change made/needed]: n/a
Attachment #8503194 - Flags: approval-mozilla-beta?
Attachment #8503194 - Flags: approval-mozilla-aurora?
Target Milestone: Firefox 35 → Firefox 36
Comment on attachment 8503191 [details] [diff] [review]
Draw divider at the bottom of about:home's tab strip (r=margaret)

Although I don't think this is a major issue for Firefox 34, this looks safe enough to take in beta2. Beta+

Lucas - FYI, in future, to keep things simple, just fill out the approval questionnaire once and flag the rest of the patches that you want to uplift.
Attachment #8503191 - Flags: approval-mozilla-beta?
Attachment #8503191 - Flags: approval-mozilla-beta+
Attachment #8503191 - Flags: approval-mozilla-aurora?
Attachment #8503191 - Flags: approval-mozilla-aurora+
Attachment #8503192 - Flags: approval-mozilla-beta?
Attachment #8503192 - Flags: approval-mozilla-beta+
Attachment #8503192 - Flags: approval-mozilla-aurora?
Attachment #8503192 - Flags: approval-mozilla-aurora+
Attachment #8503193 - Flags: approval-mozilla-beta?
Attachment #8503193 - Flags: approval-mozilla-beta+
Attachment #8503193 - Flags: approval-mozilla-aurora?
Attachment #8503193 - Flags: approval-mozilla-aurora+
Attachment #8503194 - Flags: approval-mozilla-beta?
Attachment #8503194 - Flags: approval-mozilla-beta+
Attachment #8503194 - Flags: approval-mozilla-aurora?
Attachment #8503194 - Flags: approval-mozilla-aurora+
Mike - If Lucas can't get to this soon (he's on stay-at-home-PTO), can you take a shot?
Flags: needinfo?(michael.l.comella)
Attached image Beta, w/o patch 4
I don't think we need patch 4 - this screenshot does not have the issue that the screenshot in comment 30 has (a gap below the page selection bar and the history section title, "Today").
Flags: needinfo?(michael.l.comella)
Also, the code patch 4 removes does not appear anywhere in the beta tree so I figure it was never added.
Ryan, can you land these patches on beta without patch 4 (don't use top divider...)?

Lucas, if this was incorrect of me, we can backout/fix these issues later.
Flags: needinfo?(ryanvm)
Not sure there's anything else to be done here. Canceling needinfo.
Flags: needinfo?(lucasr.at.mozilla)
History panel looks like in the attachment from comment 16, so I will mark this as verified fixed.
Builds:
Firefox for Android 34.0.1
Firefox for Android 35 Beta 10
Firefox for Android 36.0a2 (2015-01-07)

Device: Samsung Galaxy S4 (Android 4.4.2)
You need to log in before you can comment on or make changes to this bug.