Lists - Gallery layout

RESOLVED FIXED in Firefox 29

Status

()

Firefox for Android
Awesomescreen
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ibarlow, Assigned: sola)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 29
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Designs will be posted shortly.
(Reporter)

Comment 1

4 years ago
Created attachment 8345285 [details]
Sample mockup of a gallery page
(Reporter)

Updated

4 years ago
Summary: Lists - Gallery layout (phone) → Lists - Gallery layout
(Assignee)

Updated

4 years ago
Assignee: nobody → oogunsakin
(Reporter)

Comment 2

4 years ago
Created attachment 8368097 [details]
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.
(Assignee)

Comment 3

4 years ago
Created attachment 8368326 [details] [diff] [review]
bug-942889-gallery-layout.patch
Attachment #8368326 - Flags: review?(lucasr.at.mozilla)
(Assignee)

Comment 4

4 years ago
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 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
Attachment #8368326 - Flags: review?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 6

4 years ago
Created attachment 8368594 [details] [diff] [review]
bug-942889-gallery-layout.patch

-Remove unnecessary styles
-Square images
Attachment #8368326 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8368594 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8368594 - Flags: review?(lucasr.at.mozilla) → feedback+
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.
(Assignee)

Comment 9

4 years ago
Created attachment 8368635 [details] [diff] [review]
Implement changes
Attachment #8368594 - Attachment is obsolete: true
Attachment #8368635 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8368635 [details] [diff] [review]
Implement changes

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

Nice.
Attachment #8368635 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/integration/fx-team/rev/c7d8358b2d27
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c7d8358b2d27
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

Updated

4 years ago
Blocks: 968878
You need to log in before you can comment on or make changes to this bug.