900128 - Change the aspect ratio of TopBookmark thumbnails to 7:4 ratio

Status: RESOLVED FIXED

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

Description

[Sriram Ramasubramanian [:sriram]]

As per UX decision is more rectangular than square.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

OMG OMG OMG!
This is my first 900000 bug! :O :O :O :O

Comment 2

[:Margaret Leibovic]

Comment on attachment 783869 [details] [diff] [review]
Patch

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

When I installed a build with this patch applied, the top of all my old thumbnails were all cut off. Can we change how we align the images so that users will have a better experience when they upgrade to a build with the new about:home?

Giving an r- right now because of the API level issue. But it would also be nice to fix the thumbnail alignment issue here, rather than in a follow-up.

::: mobile/android/base/home/TopBookmarksView.java
@@ +163,5 @@
>  
>          // Number of rows required to show these bookmarks.
>          final int rows = (int) Math.ceil((double) total / mNumColumns);
>          final int childrenHeight = childHeight * rows;
> +        final int verticalSpacing = rows > 0 ? (rows - 1) * getVerticalSpacing() : 0;

getVerticalSpacing() is only available in API level 16:
https://developer.android.com/reference/android/widget/GridView.html#getVerticalSpacing%28%29

Is there a support API you can import?

A few more comments about all these calculations would also be good.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Oh! good catch. Let me do a work-around for this.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Going the traditional way to solve this issue.

Comment 5

[:Margaret Leibovic]

Comment on attachment 784574 [details] [diff] [review]
Patch

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

Looks good to me. Wes pointed me to bug 888507 about the thumbnails looking bad when upgrading to the new about:home, so I guess that's already a known issue.

::: mobile/android/base/home/TopBookmarksView.java
@@ +39,5 @@
>  
>      // Number of columns to show.
>      private final int mNumColumns;
>  
> +    // Vertical spacing inbetween the rows.

Nit: "in between" is two words.

Comment 6

[Wesley Johnston (:wesj)]

(In reply to :Margaret Leibovic from comment #5)
> Looks good to me. Wes pointed me to bug 888507 about the thumbnails looking
> bad when upgrading to the new about:home, so I guess that's already a known
> issue.

So this isn't fixed by this patch, right?

Comment 7

[Sriram Ramasubramanian [:sriram]]

This patch changes the aspect ratio and adds a vertical spacing.

Comment 8

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/projects/fig/rev/31027d675f80

Comment 9

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > Looks good to me. Wes pointed me to bug 888507 about the thumbnails looking
> > bad when upgrading to the new about:home, so I guess that's already a known
> > issue.
> 
> So this isn't fixed by this patch, right?

I'm not sure what problem you were seeing before, but with this patch I was seeing that the top of old thumbnails were cut off.

However, I was testing on upgrade from a recent version of fig to one with this patch applied, so we should re-test with the kind of upgrade that will happen after we merge fig to m-c. We can do that in bug 888507.

Comment 10

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #5)
> Comment on attachment 784574 [details] [diff] [review]
> Patch
> 
> Review of attachment 784574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Wes pointed me to bug 888507 about the thumbnails looking
> bad when upgrading to the new about:home, so I guess that's already a known
> issue.
> 
> ::: mobile/android/base/home/TopBookmarksView.java
> @@ +39,5 @@
> >  
> >      // Number of columns to show.
> >      private final int mNumColumns;
> >  
> > +    // Vertical spacing inbetween the rows.
> 
> Nit: "in between" is two words.

Aaah! I made this change. But forgot to qref before landing it :(

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/31027d675f80