Initial pass at Gridview visuals

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mhaigh, Assigned: mhaigh)

Tracking

unspecified
Firefox 36
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8506138 [details] [diff] [review]
WIP Patch

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

Comment 2

3 years ago
Created attachment 8506152 [details] [diff] [review]
Initial pass at Gridview visuals Initial pass at Gridview visuals
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+
(Assignee)

Comment 4

3 years ago
Created attachment 8506217 [details] [diff] [review]
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.
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+

Updated

3 years ago
Duplicate of this bug: 1072862
https://hg.mozilla.org/mozilla-central/rev/430931cea597
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
(Assignee)

Comment 9

3 years ago
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)
You need to log in before you can comment on or make changes to this bug.