Closed Bug 1076260 Opened 10 years ago Closed 10 years ago

Fix visual dividers of tabs on top

Categories

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

x86
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: antlam, Assigned: mcomella)

References

Details

Attachments

(2 files, 2 obsolete files)

Noticed these dividers were different to what we have in the design. I want to see if we can fit this in V1? I know it's a little different to what we have on desktop but wanted to give this a try. Thoughts? Not sure how we can go about changing these but I feel like the gradient that connects to the toolbar is kind of distracting. See bug 1060413 for the design.
Flags: needinfo?(michael.l.comella)
I think the design in https://bug1060413.bugzilla.mozilla.org/attachment.cgi?id=8494678 is doable. What color is the divider? And do you have specs on the size?
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Flags: needinfo?(alam)
Flags: needinfo?(michael.l.comella)
(In reply to Anthony Lam (:antlam) from comment #0) > Noticed these dividers were different to what we have in the design. I want > to see if we can fit this in V1? I know it's a little different to what we > have on desktop but wanted to give this a try. Thoughts? > > Not sure how we can go about changing these but I feel like the gradient > that connects to the toolbar is kind of distracting. > > See bug 1060413 for the design. Yeah, it should be pretty simple to change it.
Attached image spec_tabdiv1.png (obsolete) (deleted) —
Attaching specs! The div is actually #555555 (the only place we use this grey but the darkness of the background called for a new special use case color). The dimensions of it itself in my design is 30dp x 1 dp. Hope this is enough!
Flags: needinfo?(alam)
My bad! The blue is more about the width (distance from the left and right elements) and not the height. Not sure why I used that huge block to show the width...
Attached patch Adjust new tablet divider size and color (obsolete) (deleted) — Splinter Review
Lucas, the blue in the spec is the width between elements (12dp) but I'm not sure how to adjust that - to save time, do you mind taking that part (if it's not already correct)?
Attachment #8499004 - Flags: review?(lucasr.at.mozilla)
Attached image spec_tabdiv2.png (deleted) —
Attachment #8498952 - Attachment is obsolete: true
Comment on attachment 8499004 [details] [diff] [review] Adjust new tablet divider size and color Review of attachment 8499004 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with suggested changes. ::: mobile/android/base/resources/drawable-large-v11/new_tablet_tab_strip_divider.xml @@ +11,5 @@ > <size android:width="1dp" > + android:height="30dp"/> > + > + <!-- We draw this ourselves in TabStripView.draw() and to avoid implementing more > + than we have to, only bottom padding is taken into account. --> Is this comment really necessary? ::: mobile/android/base/tabs/TabStripView.java @@ +26,5 @@ > private static final String LOGTAG = "GeckoTabStrip"; > > private final TabStripAdapter adapter; > private final Drawable divider; > + // Filled by calls to ShapeDrawable.getPadding(); saved to prevent allocation in draw(). nit: add empty line before comment, break lines maybe? @@ +27,5 @@ > > private final TabStripAdapter adapter; > private final Drawable divider; > + // Filled by calls to ShapeDrawable.getPadding(); saved to prevent allocation in draw(). > + private final Rect dividerPadding; initialize directly here. @@ +144,5 @@ > @Override > public void draw(Canvas canvas) { > super.draw(canvas); > > + divider.getPadding(dividerPadding); Do you actually need to fetch padding on every draw call? The divider drawable never changes anyway. Move this to the constructor?
Attachment #8499004 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Michael Comella (:mcomella) from comment #5) > Created attachment 8499004 [details] [diff] [review] > Adjust new tablet divider size and color > > Lucas, the blue in the spec is the width between elements (12dp) but I'm not > sure how to adjust that - to save time, do you mind taking that part (if > it's not already correct)? Please file a follow-up and assign to me.
(In reply to Lucas Rocha (:lucasr) from comment #7) > > + <!-- We draw this ourselves in TabStripView.draw() and to avoid implementing more > > + than we have to, only bottom padding is taken into account. --> > > Is this comment really necessary? I like it - if you're not sure how this view is used, you'd assume the Android system takes care of it (like I did) and wonder why the padding is not working - this comment short-circuits the time wasted when you realize you're wrong and is not very invasive.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: