993749 - More padding in ArticleItemView

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

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.

Comment 1

[Lucas Rocha (:lucasr)]

(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?

Comment 2

[:Margaret Leibovic]

(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.

Comment 3

[:Margaret Leibovic]

ibarlow, do you have a mock-up of what you want this to look like?

Comment 4

[Ian Barlow (:ibarlow)]

Yes, please use the same top and bottom padding we use in our other lists, like Bookmarks: http://cl.ly/image/291b241r0a2B

Comment 5

[:Margaret Leibovic]

Attachment: 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

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.

Comment 6

[Lucas Rocha (:lucasr)]

(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 7

[Ian Barlow (:ibarlow)]

Comment on attachment 8406885 [details] [diff] [review]
More padding in ArticleItemView

Much better :)

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8406885 [details] [diff] [review]
More padding in ArticleItemView

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

Looks good to me!

Comment 9

[:Margaret Leibovic]

Attachment: 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 10

[:Margaret Leibovic]

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

Comment 11

[:Margaret Leibovic]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/788297be17eb

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/610515a58d56

Comment 14

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Attachment: 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.

Comment 16

[:Margaret Leibovic]

(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.

Comment 17

[Cristina Madaras, QA [:CristinaM]]

Based on comment 15 and comment 16 I'll mark this bug verified fixed.