1527195 - Uneven height cards for bordered card grid with 1 line title (e.g., default basic layout)

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Ed Lee :Mardak]

Attachment: Screen Shot 2019-02-11 at 5.39.52 PM.png

Looks like description flexes but single line titles cause the card to become smaller. Not sure if expected or not ?

Comment 1

[Ed Lee :Mardak]

"expected behavior is cards in a row should have the same height, "

Comment 2

[Ed Lee :Mardak]

Bisect seems to point to bug 1519879 which did change the html for DSCard

Comment 3

[Ed Lee :Mardak]

I believe it's a similar issue where the <a> is no longer the direct child of the grid parent, so when the <a> tries to flex, it flexes to the now <div> parent ??

Comment 4

[Scott [:thecount] Downe]

[Tracking Requested - why for this release]:

Comment 5

[Punam Dahiya [:pdahiya]]

https://github.com/mozilla/activity-stream/pull/4785

Comment 6

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/6d9b468951ce

Comment 7

[Kate Hudson :k88hudson]

Scott, can you add verification steps and needinfo Brahmini when you've posted them?

Comment 8

[Scott [:thecount] Downe]

Steps to verify:

1. Set pref http://browser.newtabpage.activity-stream.discoverystream.config/ to {"enabled":true,"show_spocs":true,"layout_endpoint":"https://getpocket.com/v3/newtab/layout?version=1&consumer_key=40249-e88c401e1b1f2242d9e441c4&layout_variant=dev-test-all"}
2. Load about:home
3. Look for a sponsored card that's taller than the other cards in the row.

expected: The other cards in the row's bottom should be aligned with the taller card.

I'm adding a screenshot to compare with Ed's screen shot.

Comment 9

[Scott [:thecount] Downe]

Attachment: align-bottom.png

Comment 10

[Kate Hudson :k88hudson]

Can you review the uplift and make the request?

Comment 11

[Kate Hudson :k88hudson]

Attachment: Bug 1527195 - Height issues fixed for ds card

Comment 12

[Jessilyn Davis]

Wolasi, can you QA?

Comment 13

[Wolasi :wolasi]

I have verified in Nightly 67.0a1 on Mac OS 10.14.3 by checking the height of cards in the dev-test-all layout endpoint.

Comment 14

[Tim Spurway [:tspurway]]

Comment on attachment 9044254 [details]
Bug 1527195 - Height issues fixed for ds card

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

elements on the new tab page will be improperly/awkwardly sized

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

(see comment #8)

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

this is a fairly simple patch and can be QA'd visually and doesn't introduce any new code paths

String changes made/needed

n/a

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9044254 [details]
Bug 1527195 - Height issues fixed for ds card

Planned work for pocket/new tab. Verified in Nightly.
OK for beta uplift, should land for beta 9.
Landing order: bug 1519879, bug 1525494, bug 1526861, bug 1524669, bug 1527195, bug 1525391, bug 1527347, bug 1525366, bug 1527626, bug 1527397, bug 1518258, bug 1527701, bug 1527370.

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7f0cb549e3d3

Comment 17

[Camelia Badau [:cbadau], Desktop QA]

(In reply to Wolasi from comment #13)

I have verified in Nightly 67.0a1 on Mac OS 10.14.3 by checking the height
of cards in the dev-test-all layout endpoint.

Can you please verify this issue on Firefox 66 Beta 9 (https://archive.mozilla.org/pub/firefox/candidates/66.0b9-candidates/build1/)?

Comment 18

[Wolasi :wolasi]

I have verified in Beta 66.0b9 on Mac OS 10.14.3 by checking the height of cards in the dev-test-all layout endpoint.

Comment 19

[Camelia Badau [:cbadau], Desktop QA]

(In reply to Wolasi from comment #18)

I have verified in Beta 66.0b9 on Mac OS 10.14.3 by checking the height of
cards in the dev-test-all layout endpoint.

Thanks for verifying this!