Images in article items are mis-aligned

VERIFIED FIXED in Firefox 30

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks 1 bug)

Trunk
Firefox 31
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 unaffected, firefox30 verified, firefox31 verified, firefox32 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

5 years ago
Follow-up to bug 993749.

See the screenshot, I think we should make sure the images have the same padding as the text.
Assignee

Comment 1

5 years ago
This patch is an improvement on bug 993749 and probably what I should have done originally. Instead of setting padding on the text container, this sets padding on the entire item.
Attachment #8410693 - Flags: review?(liuche)
Assignee

Comment 2

5 years ago
Posted image with patch applied
I updated my hub kitchen sink add-on to also use images: https://github.com/leibovic/hub-kitchen-sink/
Assignee

Comment 3

5 years ago
Oops, uploaded wrong patch.
Attachment #8410693 - Attachment is obsolete: true
Attachment #8410693 - Flags: review?(liuche)
Attachment #8410696 - Flags: review?(liuche)
Comment on attachment 8410696 [details] [diff] [review]
Apply padding to entire article item view

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

I like that you stuck the value in dimens.xml!

::: mobile/android/base/resources/layout/panel_article_item.xml
@@ -21,3 @@
>                    android:paddingLeft="10dip"
>                    android:paddingRight="10dip"
> -                  android:minHeight="@dimen/page_row_height"

Do we want to get rid of minHeight? I guess we'd always have some content in there...
Attachment #8410696 - Flags: review?(liuche) → review+
Assignee

Comment 5

5 years ago
(In reply to Chenxia Liu [:liuche] from comment #4)

> ::: mobile/android/base/resources/layout/panel_article_item.xml
> @@ -21,3 @@
> >                    android:paddingLeft="10dip"
> >                    android:paddingRight="10dip"
> > -                  android:minHeight="@dimen/page_row_height"
> 
> Do we want to get rid of minHeight? I guess we'd always have some content in
> there...

I got rid of the minHeight because that was something lucasr suggested in bug 993749 comment 6. As he mentions there, these items can have variable height anyway, so I think it's better to just ensure consistent top/bottom padding.
Assignee

Comment 7

5 years ago
Comment on attachment 8410696 [details] [diff] [review]
Apply padding to entire article item view

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 993749 (and new hub APIs for fx30)
User impact if declined: item padding can look inconsistent on hub panels
Testing completed (on m-c, etc.): just landed on fx-team, tested locally with kitchen sink add-on
Risk to taking this patch (and alternatives if risky): low-risk padding tweaks
String or IDL/UUID changes made by this patch: none
Attachment #8410696 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a3df9c0ee15f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8410696 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 9

5 years ago
Comment on attachment 8410696 [details] [diff] [review]
Apply padding to entire article item view

Lukas and I discussed this on IRC - I missed uplifting this for the merge, which isn't a big deal, but I will need to land it on beta after the merge is finished.
Attachment #8410696 - Flags: approval-mozilla-beta?
Attachment #8410696 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 11

5 years ago
Verified as fixed in builds:
- 32.0a1 (2014-05-05);
- 31.0a2 (2014-05-05);
Device: Google Nexus 5 (Android 4.4.2).

Will mark the bug as verified after retesting it on the next beta.

Comment 12

5 years ago
Verified as fixed in build 30 beta 2;
Device: Motorola Razr (Android 4.0.4).
Status: RESOLVED → VERIFIED
Assignee

Comment 13

5 years ago
I realized I messed up this uplift. Fixed: https://hg.mozilla.org/releases/mozilla-beta/rev/ea7fa7d12c15
You need to log in before you can comment on or make changes to this bug.