1143230 - Columns not aligned consistently on app detail pages

Status: VERIFIED FIXED

(Marketplace Graveyard :: Consumer Pages, defect, P5)

Description

[Christopher Van Wiemeersch [:cvan]]

Attachment: screenshot of grid issues on app detail page

1. Load an app detail page at desktop width. https://marketplace-dev.allizom.org/app/zirma
2. Notice how "Compatibility" and "Most Recent Version" don't line up to each other.
3. Notice how both aforementioned sections don't line up with the right column of the "Reviews" section.

Comment 1

[David Durst [:ddurst]]

We should line these up. Looks sloppy. Easy to do. Not a priority.

Comment 2

[Atique Ahmed Ziad [:atiqueahmedziad]]

I am fixing this. . :)

Comment 3

[Atique Ahmed Ziad [:atiqueahmedziad]]

I am seeing the css for every column is same. But don't understand why "Most Recent Version"  is a bit left than its upper column "Compatibility". 
However, I have noticed that if i am giving 3px padding-left to that dt & dd elements then its align with the upper column "Compatibility" .
My question is , will i write it as inline css or add a class with that elements ?

Comment 4

[David Durst [:ddurst]]

(In reply to Atique Ahmed Ziad [:atiqueahmedziad] from comment #3)
> I am seeing the css for every column is same. But don't understand why "Most
> Recent Version"  is a bit left than its upper column "Compatibility". 
> However, I have noticed that if i am giving 3px padding-left to that dt & dd
> elements then its align with the upper column "Compatibility" .
> My question is , will i write it as inline css or add a class with that
> elements ?

[Apologies for the delayed response!]

You should avoid inline css, as it makes it harder to find all the styles being applied. You should do all of this via selectors and declarations in .styl files.

It does seem like the styling is slightly different from the (mostly similar) container above it without a lot of purpose -- though the error itself may be a quirk.

My opinion: it seems like the width for the <dl>s are probably better specified as 50% (and corrected to display block when the viewport width changes to something more narrow), since that's really what's desired. Then the box model settings can be virtually the same as those of the <li>s in the Reviews section above it.

Admittedly, when I did this, I still saw the <dl>s stacked, even though they were set to display inline-block, until I set them to display block and float left (which is probably fine too); only then was the alignment completely correct.

Comment 5

[Mark Striemer [:mstriemer]]

You can get the <dl>s to avoid stacking by setting `width: 50%` on the `dl` and `font-size: 0;` on `.app-more-metadata`. The `dl` should also have `padding: 0 12px` like the reviews above and `app-more-metadata` will need a negative margin to account for that so it stays left aligned.

I'm not sure how that will get handled as things get more narrow but it lines it up on desktop.

Comment 6

[Atique Ahmed Ziad [:atiqueahmedziad]]

Attachment: box alignment okey.png

Thanks David & Mark for your reply. 

I have set width:50%; float:left; display: block; .Then the forth box alignment is okey. Check the attachment. :)

Comment 7

[Atique Ahmed Ziad [:atiqueahmedziad]]

Attachment: align with review.png

And Mark, when I am giving padding: 0 12px; to .app-more-metadata . Then looks like the following attachment. 
It like it has match the right side review alignment but moved from left side. Will I set a class in those of two 'dl' which are at the right side and there i will set padding-left:12px; . Please check the attachment.

Comment 8

[Mark Striemer [:mstriemer]]

Looks good so far! If you set `.app-more-metadata { margin: 0 -12px; }` that should fix the first columns alignment.

Comment 9

[Atique Ahmed Ziad [:atiqueahmedziad]]

Attachment: pull request

Please review :)

Comment 10

[Mark Striemer [:mstriemer]]

https://github.com/mozilla/fireplace/commit/ecb6857fe12c057d0f022f4b93ff64634a9b910e

Thanks for the patch!

Comment 11

[Madalin Cotetiu]

Verified as fixed in FF41(Win7) in marketplace-dev.allizom.org
Postfix screencast: http://screencast.com/t/ka8iWwU7mkl
Closing bug.