1076898 - Initial pass at Gridview visuals

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martyn Haigh (:mhaigh)]

Spruce up the visuals for the new gridview according to https://bugzilla.mozilla.org/show_bug.cgi?id=1064415.  It doesn't matter if things aren't exact for this first pass as we'll revisit later on and polish near the end of the project.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: WIP Patch

Looking good on N7 - some tweaks needed to 10 inch tablet

Comment 2

[Martyn Haigh (:mhaigh)]

Attachment: Initial pass at Gridview visuals Initial pass at Gridview visuals

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8506152 [details] [diff] [review]
Initial pass at Gridview visuals Initial pass at Gridview visuals

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

This is looking pretty good overall.

::: mobile/android/base/resources/values-land/dimens.xml
@@ +8,4 @@
>      <!-- Remote Tabs static view top padding. Less in landscape on phones. -->
>      <dimen name="home_remote_tabs_top_padding">16dp</dimen>
>  
> +    <dimen name="new_tab_grid_padding">48dp</dimen>

Ditto: rename to new_tablet_tab_panel_grid_padding

::: mobile/android/base/resources/values/dimens.xml
@@ +117,5 @@
>      <dimen name="history_tab_indicator_height">50dp</dimen>
>  
> +
> +    <dimen name="new_tab_thumbnail_height">180dp</dimen>
> +    <dimen name="new_tab_thumbnail_width">180dp</dimen>

For consistency:
new_tab_thumbnail_height -> new_tablet_tab_thumbnail_height
new_tab_thumbnail_width -> new_tablet_tab_thumbnail_width
new_tab_grid_padding -> new_tablet_tab_panel_grid_padding

@@ +118,5 @@
>  
> +
> +    <dimen name="new_tab_thumbnail_height">180dp</dimen>
> +    <dimen name="new_tab_thumbnail_width">180dp</dimen>
> +    <dimen name="new_tab_grid_padding">24dp</dimen>

Why do need to set padding on the cell?

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +67,5 @@
> +        setGravity(Gravity.CENTER);
> +        setNumColumns(GridView.AUTO_FIT);
> +
> +        Resources r = getResources();
> +        float columnWidth = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_PX, r.getDimension(R.dimen.new_tab_thumbnail_width), r.getDisplayMetrics());

You can just use r.getDimensionPixelSize() instead.

@@ +70,5 @@
> +        Resources r = getResources();
> +        float columnWidth = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_PX, r.getDimension(R.dimen.new_tab_thumbnail_width), r.getDisplayMetrics());
> +        setColumnWidth((int) columnWidth);
> +
> +        float padding = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_PX, r.getDimension(R.dimen.new_tab_grid_padding), r.getDisplayMetrics());

Ditto.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Initial pass at Gridview visuals Initial pass at Gridview visuals

We're not setting the padding on the cell, but on the gridview itself.  The padding gives us the space at the sides between the cells and the edge of the screen.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8506217 [details] [diff] [review]
Initial pass at Gridview visuals Initial pass at Gridview visuals

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

Go.

::: mobile/android/base/resources/layout/new_tablet_tabs_item_cell.xml
@@ +13,5 @@
> +                                           android:paddingBottom="6dip"
> +                                           android:paddingLeft="1dip"
> +                                           android:paddingRight="1dip"
> +                                           android:gravity="center"
> +                                           android:orientation="vertical">

Does this show the background highlight on pressed state? File a follow-up if it doesn't.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +66,5 @@
> +        setStretchMode(GridView.STRETCH_SPACING);
> +        setGravity(Gravity.CENTER);
> +        setNumColumns(GridView.AUTO_FIT);
> +
> +        Resources r = getResources();

nit: r -> resources for clarity. Make it final.

@@ +67,5 @@
> +        setGravity(Gravity.CENTER);
> +        setNumColumns(GridView.AUTO_FIT);
> +
> +        Resources r = getResources();
> +        float columnWidth = r.getDimensionPixelSize(R.dimen.new_tablet_tab_thumbnail_width);

final

@@ +70,5 @@
> +        Resources r = getResources();
> +        float columnWidth = r.getDimensionPixelSize(R.dimen.new_tablet_tab_thumbnail_width);
> +        setColumnWidth((int) columnWidth);
> +
> +        float padding = r.getDimensionPixelSize(R.dimen.new_tablet_tab_panel_grid_padding);

final

@@ +71,5 @@
> +        float columnWidth = r.getDimensionPixelSize(R.dimen.new_tablet_tab_thumbnail_width);
> +        setColumnWidth((int) columnWidth);
> +
> +        float padding = r.getDimensionPixelSize(R.dimen.new_tablet_tab_panel_grid_padding);
> +        setPadding((int) padding, 0, (int) padding, 0);

In this casting really needed?

Comment 6

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/430931cea597

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/430931cea597

Comment 9

[Martyn Haigh (:mhaigh)]

Can I get some UX feedback on this initial pass?  Prob worth making a new bug if there are things to track.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Martyn Haigh (:mhaigh) from comment #9)
> Can I get some UX feedback on this initial pass?  Prob worth making a new
> bug if there are things to track.

Let's clean up our bugs again and leave this one closed.

We can pick up fedback in bug 1064415.