942889 - Lists - Gallery layout

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Ian Barlow (:ibarlow)]

Designs will be posted shortly.

Comment 1

[Ian Barlow (:ibarlow)]

Attachment: Sample mockup of a gallery page

Comment 2

[Ian Barlow (:ibarlow)]

Attachment: gallery guidelines.png

Here are some guidelines on how to scale and crop images in the gallery. 

One thing of note: let's use slightly larger default images on larger tablets, as it will look a little better that way.

Comment 3

[Sola Ogunsakin [:sola]]

Attachment: bug-942889-gallery-layout.patch

Comment 4

[Sola Ogunsakin [:sola]]

Comment on attachment 8368326 [details] [diff] [review]
bug-942889-gallery-layout.patch

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

::: mobile/android/base/resources/raw/fake_home_items.json
@@ +9,5 @@
>      "dataset_id": "fake-dataset",
>      "url": "http://example.com/second",
>      "title": "Second Example",
>      "description": "This is a second example"
> +}, ]

will fix

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8368326 [details] [diff] [review]
bug-942889-gallery-layout.patch

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

Looking good. Need to fix the style duplication.

::: mobile/android/base/home/PanelGridItemView.java
@@ +48,4 @@
>      }
>  
>      public void updateFromCursor(Cursor cursor) { }
> +}
\ No newline at end of file

Unrelated, revert.

::: mobile/android/base/resources/values/colors.xml
@@ +89,5 @@
>    <color name="url_bar_shadow">#12000000</color>
>  
>    <color name="home_last_tab_bar_bg">#FFF5F7F9</color>
>  
> +  <color name="panel_grid_item_image_background">#D1D9E1</color>

Is that a color that ibarlow suggested?

::: mobile/android/base/resources/values/styles.xml
@@ +144,5 @@
>        <item name="android:padding">5dip</item>
>        <item name="android:orientation">vertical</item>
>      </style>
>  
> +    <style name="Widget.PanelGridView" parent="Widget.GridView">

You don't need to replicate the styles on every configuration. The only thing that changes across configurations is the columnWidth. So, define the styles only once in here (values/styles.xml) and create a dimension value (i.e. in the dimens.xml files) that defines the value per configuration (180dp for phones, 250dp for tablets).

Generally speaking:
- Same layout, slightly different paddings, margins, alignment -> vary on styles
- Same styles, slightly different diferent padding, margins -> vary on dimensions

Comment 6

[Sola Ogunsakin [:sola]]

Attachment: bug-942889-gallery-layout.patch

-Remove unnecessary styles
-Square images

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8368594 [details] [diff] [review]
bug-942889-gallery-layout.patch

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

Looks nice. Just needs to fix the redundant bits.

::: mobile/android/base/resources/values-large-land-v11/dimens.xml
@@ +5,5 @@
>  
>  <resources>
>  
>      <dimen name="history_tab_widget_width">360dp</dimen>
> +    <dimen name="panel_grid_view_column_width">250dp</dimen>

This one is redudant as you've already defined it in values-large-v11/dimens.xml. Remove.

::: mobile/android/base/resources/values-xlarge-land-v11/dimens.xml
@@ +5,5 @@
>  
>  <resources>
>  
>      <dimen name="history_tab_widget_width">480dp</dimen>
> +    <dimen name="panel_grid_view_column_width">250dp</dimen>

Same here, values-large-v11/dimens.xml is enough to cover both small and large tablets on all orientations. Remove.

::: mobile/android/base/resources/values-xlarge-v11/dimens.xml
@@ +11,5 @@
>      <dimen name="tabs_counter_size">26sp</dimen>
>      <dimen name="tabs_panel_indicator_width">60dp</dimen>
>      <dimen name="tabs_panel_list_padding">8dip</dimen>
>      <dimen name="history_tab_widget_width">270dp</dimen>
> +    <dimen name="panel_grid_view_column_width">250dp</dimen>

Ditto, remove.

::: mobile/android/base/resources/values/styles.xml
@@ +148,5 @@
> +    <style name="Widget.PanelGridView" parent="Widget.GridView">
> +        <item name="android:paddingTop">0dp</item>
> +        <item name="android:stretchMode">columnWidth</item>
> +        <item name="android:layout_width">fill_parent</item>
> +        <item name="android:layout_height">fill_parent</item>

nit: move layout_* to the top.

@@ +155,5 @@
> +        <item name="android:verticalSpacing">2dp</item>
> +    </style>
> +
> +    <style name="Widget.PanelGridItemView">
> +      <item name="android:orientation">vertical</item>

PanelGridItemView is a FrameLayout, no orientation.

Comment 8

[Lucas Rocha (:lucasr)]

After having a look at the design specs, you should actually define the (columnwidth = 250dp) dimension in values-xlarge-v11/dimens.xml (instead of values-large-v11) as it only applies to large tablets.

Comment 9

[Sola Ogunsakin [:sola]]

Attachment: Implement changes

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8368635 [details] [diff] [review]
Implement changes

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

Nice.

Comment 11

[Lucas Rocha (:lucasr)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c7d8358b2d27