Style not showing correctly for incompatible add-on placeholders in featured add-ons box

VERIFIED FIXED in 5.0.6

Status

VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: davemgarrett, Assigned: clouserw)

Tracking

({polish})

unspecified
5.0.6
polish

Details

(URL)

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
Created attachment 381552 [details]
screenshots

View the front page in a recent nightly and you'll see a whole host of "This add-on is for older versions of Firefox" placeholders. Compare the style of the ones in the featured box up top and the recommended and popular panes in the box below. (see attached) The ones below look nice but up top the text is misaligned to the left instead of centered and it's missing the green coloring for recommended status. The "Recommended" label is gone too, but that was probably intentional. Also note that the experimentals up here are missing their red/pink coloring as well (see bug 496364).
Can we please 5.0.6 this?  Looking crappy in IE is one thing, but I want to look great in Firefox, at least.
Target Milestone: --- → 5.0.6
(Assignee)

Comment 2

9 years ago
Created attachment 381712 [details] [diff] [review]
add flags

This fixes this bug and bug 496364.  The only thing I haven't figured out is the button in RTL is off center, but only in this area - the other buttons on the page are fine.  Seems like it should be obvious but I think I've been up too long.

I'm sure Ehsan or rdoherty will know why
Attachment #381712 - Flags: review?(rdoherty)
(Assignee)

Comment 3

9 years ago
Created attachment 381713 [details]
example of broken rtl button after this patch

Comment 4

9 years ago
Created attachment 381743 [details] [diff] [review]
RTL fix

This patch should fix the RTL problem.  With this rule in main-mozilla.css while the original rule in main.css, it overrides some other rules in main.css for RTL locales.  In this case, the rule which is mistakenly overridden is:

.section-teaser .addon-view p,
.section-teaser .addon-view h3 {
    margin-left: 0;
    margin-right: 0;
    line-height: 1.23em;
}

(the margin-left and margin-right rules are overridden of course).
Attachment #381743 - Flags: review?(rdoherty)

Updated

9 years ago
Attachment #381712 - Flags: review?(rdoherty) → review+
Comment on attachment 381712 [details] [diff] [review]
add flags

That looks good.
Comment on attachment 381743 [details] [diff] [review]
RTL fix

No, this does not fix the bug unfortunately. The problem is not the rule being overwritten. The problem is the rule itself.

If we need it to stay, so that the add-on descriptions in that box have a proper margin, you probably need to add a rule for

.html-rtl .section-teaser .column p.install-button

and remove the margin-right there.
Attachment #381743 - Flags: review?(rdoherty) → review-

Comment 7

9 years ago
I tried to apply the effects of Wil's patch inside Firebug and go from there, but I may have easily been misguided.  Maybe we can land this and file a follow-up for the RTL case?  Once it shows up on preview I can prepare a working patch, I'm sure.
(Assignee)

Comment 8

9 years ago
Thanks wenzel.  This is in r27225.  Leaving open for Ehsan's patch.
Assignee: nobody → clouserw

Comment 9

9 years ago
Comment on attachment 381743 [details] [diff] [review]
RTL fix

(In reply to comment #6)
> (From update of attachment 381743 [details] [diff] [review])
> No, this does not fix the bug unfortunately. The problem is not the rule being
> overwritten. The problem is the rule itself.

I investigated this deeply, and while I don't have an AMO instance running locally, I really think that my patch should fix this issue...

> If we need it to stay, so that the add-on descriptions in that box have a
> proper margin, you probably need to add a rule for
> 
> .html-rtl .section-teaser .column p.install-button
> 
> and remove the margin-right there.

What is happening is that we have the rule |.section-teaser .column p, .section-teaser .column h3| in main.css which tries to set a left margin to p and h3 elements inside the teaser columns, but then again we have another rule |.section-teaser .addon-view p, .section-teaser .addon-view h3| which AFAICT overrides the original rule in every case, since all .column's are in .addon-view li's.

Now, we have a |.html-rtl .section-teaser .column p, .html-rtl .section-teaser .column h3| after both these rules in main-mozilla.css which tries to reverse the |.section-teaser .column p, .section-teaser .column h3| rule for RTL mode, but also overrides |.section-teaser .addon-view p, .section-teaser .addon-view h3| which sets left/right margins to 0.  This causes the correct zero margins not to take affect.

My patch moves the RTL rule to exactly after the first rule to make it correct for RTL mode (even though the first rule seems useless), and therefore the |.section-teaser .addon-view p, .section-teaser .addon-view h3| should kick in for RTL mode exactly like LTR mode.
Attachment #381743 - Flags: review- → review?(fwenzel)

Updated

9 years ago
Attachment #381743 - Flags: review?(fwenzel) → review-
Comment on attachment 381743 [details] [diff] [review]
RTL fix

I think what you are missing is that the rule you are moving takes effect all the time, no matter where you put it, as it matches "better" than the non-.html-rtl rule. Here's what fixed it:

diff --git a/site/app/webroot/css/amo2009/main-mozilla.css b/site/app/webroot/css/amo2009/main-mozilla.css
index 17f30ac..359e5e6 100644
--- a/site/app/webroot/css/amo2009/main-mozilla.css
+++ b/site/app/webroot/css/amo2009/main-mozilla.css
@@ -194,6 +194,9 @@ p.install-button {
     margin-left: 0;
     margin-right: 1em;
 }
+.html-rtl .section-teaser .column p.install-button {
+    margin-right: 0;
+}
 .html-rtl .section-teaser .teaser-header ol li {
     margin-left: 5em;
     margin-right: 0;

I'll check that in like this if that's all right with you.
r27249 -- Ehsan, can you verify the bug please, one preview picks it up? Thanks!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: push-needed
Resolution: --- → FIXED

Comment 12

9 years ago
(In reply to comment #11)
> r27249 -- Ehsan, can you verify the bug please, one preview picks it up?
> Thanks!

Yes, it's fixed now.  Thanks!
This fix made my day, I want you to know.

Verified FIXED.
Status: RESOLVED → VERIFIED
removing "push-needed" from 105 AMO 5.0.6 bugs; filter on "I hate stephend!"
Keywords: push-needed
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.