Closed Bug 1084098 Opened 7 years ago Closed 7 years ago

Adjust padding/margins of Sync tabs panel items

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: antlam, Assigned: nalexander)

References

Details

Attachments

(4 files)

To allow for more visual distinction, I'd like to adjust the padding and margins of the list items. Namely, since both list items follow a similar layout structure, using some size and padding to separate them a bit more visually.

Attaching spec. (My bad I didn't realize I hadn't done this in the previous bugs).

Let me know if there are any issues!
NI-ing Nick
Flags: needinfo?(nalexander)
Blocks: remotetabsv2
Flags: needinfo?(nalexander)
(In reply to Anthony Lam (:antlam) from comment #0)
> Created attachment 8506475 [details]
> prev_mob_syncpanel_tabs6_spec.png
> 
> To allow for more visual distinction, I'd like to adjust the padding and
> margins of the list items. Namely, since both list items follow a similar
> layout structure, using some size and padding to separate them a bit more
> visually.
> 
> Attaching spec. (My bad I didn't realize I hadn't done this in the previous
> bugs).
> 
> Let me know if there are any issues!

I have issues with the spec and then issues with the tech.  Tech first.

Right now, the layouts of the individual tabs are exactly the same as the layouts of bookmarks, history items, etc -- including the "Switch to tab" functionality, etc.  I really, really, really want to keep these the same because we share code (TwoLinePageRow) and the distinctive visual identity of the home pager.  We can find a way to generalize the layout, but it's complexity for little gain.

As for the spec, the measurements are all over the place: we have measurements of 40, 56, 64, 5, 10, 15, and 25 dp.  And of course, the differences between them all.   The three different row heights don't make sense to me.  Things don't align: the icons are centered, but their centers are awkwardly offset; and the space to the right of the tab text is cut off, but not in the center of the arrow icons above the text.

We can make this happen -- I did the group/client changes before running into the TwoLinePageRow sharing -- if we have to.  But I suggest that we keep the item/tab rows the same and critically rethink the strategy for the group/clients.  antlam?
Flags: needinfo?(alam)
(In reply to Nick Alexander :nalexander from comment #2)
> I have issues with the spec and then issues with the tech.  Tech first.
> 
> Right now, the layouts of the individual tabs are exactly the same as the
> layouts of bookmarks, history items, etc -- including the "Switch to tab"
> functionality, etc.  I really, really, really want to keep these the same
> because we share code (TwoLinePageRow) and the distinctive visual identity
> of the home pager.  We can find a way to generalize the layout, but it's
> complexity for little gain.

Don't worry. I want to keep these the same too. I am just looking to update them. Bug 1071887 has an example of this effort on the History panel, and the latest design for the Sync Tabs panel follows the same visual language. The way we handle page title, url, favicon, and possible bookmark star, etc... are the same in all the latest designs for the panels.

Creating a style for a "group" item in the panel gives us the long term gain of having a style for "group" items in our panel UI that we can re-use later down the line (which we probably will need). 

Color is the only distinction between a "group" and an "item" ATM and I do not think this is a great user experience as they start to awkwardly compete with each other when they really shouldn't. 
 
> As for the spec, the measurements are all over the place: we have
> measurements of 40, 56, 64, 5, 10, 15, and 25 dp.  And of course, the
> differences between them all. 

I'm open to cleaning up the 5dp on the left to a 6dp so that when we add the clearance space for the device icon (50dp), it adds up to 56dp (the height of "group" items).

I'm reluctant about the 4dp because that's just the most comfortable spacer between Page title and URL that I've found. We should be using that in all these types of list items.

5, 10, 15, 25 are pretty good jumps in units actually. Basically, creating a grid gives us these units. I'm not sure that having more units of padding or what not is a reason we should avoid making new things but I agree that we should make sure if we're breaking any sort of grid/pattern, that it's for a reason.
  
> The three different row heights don't make sense to me.  

There are generally 3 items when a user looks at this panel. 1) A URL/tab, 2) a group/device, 3) hidden devices that they've chosen to not want to see right now. In that order of importance, I have designed for 3 sizes that are derived from the Android guidelines (64dp is the standard row height for a list item's hit area) so that they do not awkwardly compete with each other.

> Things don't align: the icons are centered, but their centers
> are awkwardly offset; and the space to the right of the tab text is cut off,
> but not in the center of the arrow icons above the text.

On the note of breaking that visual grid purposefully, I've deliberately leveraged the different heights of these row items to give them different offsets so they break the grid (again) and so they stand out as separate visually.

The text cut off should be at the center of the arrows since both the arrow asset is 15dp and the padding is as well (15dp + 15dp = 30dp). The URL text should cut off at approximately half way of the arrow asset (of course dependent on the text itself).

> We can make this happen -- I did the group/client changes before running
> into the TwoLinePageRow sharing -- if we have to.  But I suggest that we
> keep the item/tab rows the same and critically rethink the strategy for the
> group/clients.  antlam?

Not sure if that answers your questions about consistency and strategy but that's the reasoning for creating "three different row heights".

What do you mean by keeping the "item/tab rows the same"?
Flags: needinfo?(alam)
In editors (Eclipse and IntelliJ), Android View sub-classes are created
in a "Bridging" mock context.  There is no ambient GeckoApplication in
this case.  It's already okay for the LWT to be null; let's loosen the
requirement that there be an ambient GeckoApplication to let these View
sub-classes render in editors.
Attachment #8530428 - Flags: review?(michael.l.comella)
Pretty straight-forward implementation of the mock.  The mock shows a
different "bookmark star" icon, with slightly different right-margins,
and implementing that change requires changing the TwoLinePageRow layout
to not use drawableRight for the star.  (Otherwise, the star will not be
vertically centered in the container.)  I don't care to make that change
for this ticket.
Attachment #8530429 - Flags: review?(michael.l.comella)
These tags are used by Android tools (including IntelliJ) at design-time
to show "sample data".  They're stripped entirely at build time.
Attachment #8530430 - Flags: review?(michael.l.comella)
Attachment #8530428 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8530430 [details] [diff] [review]
Add Android tools tags to some Remote Tabs home panel views.

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

I am just assuming how the "tools:" attribute works, but assuming it works how I assume it does, lgtm.
Attachment #8530430 - Flags: review?(michael.l.comella) → review+
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment on attachment 8530429 [details] [diff] [review]
Adjust padding/margins of Sync tabs panel items.

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

r+ w/ an answer to my question.

::: mobile/android/base/resources/layout/home_remote_tabs_group.xml
@@ +11,4 @@
>      android:gravity="center_vertical"
> +    android:layout_width="match_parent"
> +    android:layout_height="@dimen/page_group_height"
> +    android:minHeight="@dimen/page_group_height" >

Why did you specify both layout_height and minHeight here?

nit: If you're touching this line anyway, remove the space. (While I'm not very opinionated over it, it appears you've done this on other lines below)
Attachment #8530429 - Flags: review?(michael.l.comella) → review+
(In reply to Michael Comella (:mcomella) from comment #8)
> Comment on attachment 8530429 [details] [diff] [review]
> Adjust padding/margins of Sync tabs panel items.
> 
> Review of attachment 8530429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ w/ an answer to my question.
> 
> ::: mobile/android/base/resources/layout/home_remote_tabs_group.xml
> @@ +11,4 @@
> >      android:gravity="center_vertical"
> > +    android:layout_width="match_parent"
> > +    android:layout_height="@dimen/page_group_height"
> > +    android:minHeight="@dimen/page_group_height" >
> 
> Why did you specify both layout_height and minHeight here?

Purely cargo culted from two_line_page_row, IIRC.  I *think* one is outward facing -- layout_height -- and the other is inward facing -- minHeight.  That is, the former impacts the container's layout, and the latter impacts the child's layout.  Could be unnecessary, but I don't care to find out.

> nit: If you're touching this line anyway, remove the space. (While I'm not
> very opinionated over it, it appears you've done this on other lines below)

Roger that.
Hardware: x86 → All
Flags: needinfo?(alam)
Awesome, looks good Nick! thanks! :)
Flags: needinfo?(alam)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.