Closed Bug 993749 Opened 6 years ago Closed 6 years ago

More padding in ArticleItemView

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox30 --- verified
firefox31 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This was feedback from ibarlow about my Home Feeds add-on, but it's an issue we'll need to fix in the product.

Right now we have 5dip padding on the text in the view:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/panel_article_item.xml#19

I tried comparing this layout to the layouts we use for our built-in panels, and it looks like we use a min height for those items, maybe we should do something similar here.
(In reply to :Margaret Leibovic from comment #0)
> This was feedback from ibarlow about my Home Feeds add-on, but it's an issue
> we'll need to fix in the product.
> 
> Right now we have 5dip padding on the text in the view:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> layout/panel_article_item.xml#19
> 
> I tried comparing this layout to the layouts we use for our built-in panels,
> and it looks like we use a min height for those items, maybe we should do
> something similar here.

Not sure I follow. We do set a min height for ArticleItemView text:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/panel_article_item.xml#23

How does min height relate to padding here?
(In reply to Lucas Rocha (:lucasr) from comment #1)
> (In reply to :Margaret Leibovic from comment #0)
> > This was feedback from ibarlow about my Home Feeds add-on, but it's an issue
> > we'll need to fix in the product.
> > 
> > Right now we have 5dip padding on the text in the view:
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> > layout/panel_article_item.xml#19
> > 
> > I tried comparing this layout to the layouts we use for our built-in panels,
> > and it looks like we use a min height for those items, maybe we should do
> > something similar here.
> 
> Not sure I follow. We do set a min height for ArticleItemView text:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> layout/panel_article_item.xml#23
> 
> How does min height relate to padding here?

My guess was that the min height for the TwoLinePageRow items was resulting in the appearance of more padding (e.g. if content + padding < min height), but you're right, I missed that min height style declaration in the article item layout.

So yeah, we probably just need more padding in these items.
Assignee: nobody → margaret.leibovic
ibarlow, do you have a mock-up of what you want this to look like?
Flags: needinfo?(ibarlow)
Yes, please use the same top and bottom padding we use in our other lists, like Bookmarks: http://cl.ly/image/291b241r0a2B
Flags: needinfo?(ibarlow)
So right now, for the other lists, the "padding" is determined by the min height of the rows. Although our article items have the same min height, it doesn't have the same effect because the description can wrap to 2 rows. So here I just added some extra top/bottom padding to the text container.

Here's what it looks like with some items with various description lengths:
https://dl.dropboxusercontent.com/u/3358452/2014-04-15%2015.05.10.png

Part of me thinks we should refactor our two line page row styles so that we can have matching styles, but part of me also just wants a quick fix here that we can uplift to Aurora. So maybe we can file a follow-up bug about some refactoring.
Attachment #8406885 - Flags: review?(liuche)
Attachment #8406885 - Flags: feedback?(ibarlow)
(In reply to :Margaret Leibovic from comment #5)
> Created attachment 8406885 [details] [diff] [review]
> More padding in ArticleItemView
> 
> So right now, for the other lists, the "padding" is determined by the min
> height of the rows. Although our article items have the same min height, it
> doesn't have the same effect because the description can wrap to 2 rows. So
> here I just added some extra top/bottom padding to the text container.
> 
> Here's what it looks like with some items with various description lengths:
> https://dl.dropboxusercontent.com/u/3358452/2014-04-15%2015.05.10.png

You probably want to test other variations: multi-line title with single-line description, multi-line title with no description, multi-line description with no title, etc.

As you said, article views are different than TwoLineRows in that they have variable heights. So, maybe there shouldn't be a minimum height at all. We just need to ensure that all rows have consistent top/bottom padding.
Comment on attachment 8406885 [details] [diff] [review]
More padding in ArticleItemView

Much better :)
Attachment #8406885 - Flags: feedback?(ibarlow) → feedback+
Comment on attachment 8406885 [details] [diff] [review]
More padding in ArticleItemView

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

Looks good to me!
Attachment #8406885 - Flags: review?(liuche) → review+
Attached image screenshot
Lucas's comment inspired me to make a kitchen sink add-on to test ALL THE APIS! As part of this we can test all sorts of data combinations.

https://github.com/leibovic/hub-kitchen-sink
Comment on attachment 8406885 [details] [diff] [review]
More padding in ArticleItemView

[Approval Request Comment]
Bug caused by (feature/regressing bug #): part of new hub APIs (being promoted in fx30)
User impact if declined: home panel list looks a bit squished
Testing completed (on m-c, etc.): just landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk padding tweak
String or IDL/UUID changes made by this patch: none
Attachment #8406885 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/788297be17eb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8406885 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 999760
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
Device: LG Nexus 4 (Android 4.4.2)
Builds: 30 beta 8, 31 Aurora and 32 Nightly 01/06
Installing kitchen-sink (https://github.com/leibovic/hub-kitchen-sink): 
Test List panel appears as in the screenshot, but Test Grid appears different in Nightly and Beta.
Please look at the screenshot.
(In reply to Teodora Vermesan (:TeoVermesan) from comment #15)
> Created attachment 8432461 [details]
> Screenshot from 2014-06-02 15:18:28.png
> 
> Device: LG Nexus 4 (Android 4.4.2)
> Builds: 30 beta 8, 31 Aurora and 32 Nightly 01/06
> Installing kitchen-sink (https://github.com/leibovic/hub-kitchen-sink): 
> Test List panel appears as in the screenshot, but Test Grid appears
> different in Nightly and Beta.
> Please look at the screenshot.

The grid view is created differently. The difference is likely due to bug 974454, which I don't think we need to uplift.
Based on comment 15 and comment 16 I'll mark this bug verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.