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.

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?

(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.

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!

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...

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)?

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?

(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.

Address nits.