Closed Bug 1076898 Opened 10 years ago Closed 10 years ago

Initial pass at Gridview visuals

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch WIP Patch (obsolete) — Splinter Review
Looking good on N7 - some tweaks needed to 10 inch tablet
Attachment #8506138 - Attachment is obsolete: true
Attachment #8506152 - Flags: feedback?(lucasr.at.mozilla)
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.
Attachment #8506152 - Flags: feedback?(lucasr.at.mozilla) → feedback+
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.
Attachment #8506152 - Attachment is obsolete: true
Attachment #8506217 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8506217 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/430931cea597
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Can I get some UX feedback on this initial pass?  Prob worth making a new bug if there are things to track.
Flags: needinfo?(alam)
(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.
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.