Closed
Bug 999760
Opened 10 years ago
Closed 10 years ago
Images in article items are mis-aligned
Categories
(Firefox for Android Graveyard :: Theme and Visual Design, defect)
Tracking
(firefox29 unaffected, firefox30 verified, firefox31 verified, firefox32 verified)
VERIFIED
FIXED
Firefox 31
Tracking | Status | |
---|---|---|
firefox29 | --- | unaffected |
firefox30 | --- | verified |
firefox31 | --- | verified |
firefox32 | --- | verified |
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(3 files, 1 obsolete file)
272.27 KB,
image/png
|
Details | |
247.94 KB,
image/png
|
Details | |
3.38 KB,
patch
|
liuche
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•10 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•10 years ago
|
||
I updated my hub kitchen sink add-on to also use images: https://github.com/leibovic/hub-kitchen-sink/
Assignee | ||
Comment 3•10 years ago
|
||
Oops, uploaded wrong patch.
Attachment #8410693 -
Attachment is obsolete: true
Attachment #8410693 -
Flags: review?(liuche)
Attachment #8410696 -
Flags: review?(liuche)
Comment 4•10 years ago
|
||
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•10 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 6•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a3df9c0ee15f
Assignee | ||
Comment 7•10 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?
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a3df9c0ee15f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•10 years ago
|
Attachment #8410696 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 9•10 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?
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8410696 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/d9336eebf796
Comment 11•10 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.
status-firefox32:
--- → verified
Comment 12•10 years ago
|
||
Verified as fixed in build 30 beta 2; Device: Motorola Razr (Android 4.0.4).
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 13•10 years ago
|
||
I realized I messed up this uplift. Fixed: https://hg.mozilla.org/releases/mozilla-beta/rev/ea7fa7d12c15
Updated•3 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
•