Closed
Bug 942889
Opened 12 years ago
Closed 11 years ago
Lists - Gallery layout
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 29
People
(Reporter: ibarlow, Assigned: oogunsakin)
References
Details
Attachments
(3 files, 2 obsolete files)
Designs will be posted shortly.
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Summary: Lists - Gallery layout (phone) → Lists - Gallery layout
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → oogunsakin
Reporter | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Attachment #8368326 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 4•11 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 5•11 years ago
|
||
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•11 years ago
|
||
-Remove unnecessary styles
-Square images
Attachment #8368326 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8368594 -
Flags: review?(lucasr.at.mozilla)
Comment 7•11 years ago
|
||
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+
Comment 8•11 years ago
|
||
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•11 years ago
|
||
Attachment #8368594 -
Attachment is obsolete: true
Attachment #8368635 -
Flags: review?(lucasr.at.mozilla)
Comment 10•11 years ago
|
||
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+
Comment 11•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Updated•11 years ago
|
Keywords: checkin-needed
Comment 12•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•