fix(client): give sizes="auto" card images their intrinsic aspect ratio - #7973
Conversation
`getCardImageSrcSetProps` hands every card image `sizes="auto"` plus `loading="lazy"`, but no `width`/`height`. Without that pair an `<img>` has no intrinsic aspect ratio of its own, so the spec's 300x150 default object size stands in and any element that lets the image supply its own height lays out a 2:1 box. `object-cover` then crops the card to a middle slice. Measured in Chromium 151 with the shipped stylesheet, the desktop hover preview's markup lays out 472x150 with the attribute set and 472x658 without it. Five image sites derive their height from the image and were letterboxed: the desktop hover preview, both mobile preview overlays (`CardPreview`), the textbox slice (`CardTextboxPreview`, whose `translateY` offset is a percentage of that height), and the coverage dashboard. The other fifteen call sites constrain both axes in CSS and were unaffected — CSS still wins there, so the pair only ever supplies the ratio nothing else provides. Fixed in the helper rather than at the five call sites: the helper is what introduces `sizes="auto"`, so it owes every consumer the intrinsic size that keyword resolves against. Both rungs are the same card scan at two widths, so pinning the `normal` rung's 488x680 — the ratio the client's `aspect-[488/680]` placeholder tiles already encode — fixes the ratio for the whole ladder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughChangesThe card image source-set contract now includes intrinsic Card image intrinsic dimensions
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change gives auto-sized card images their intended intrinsic aspect ratio, restoring correct card rendering without changing data flow, permissions, dependencies, or deployment behavior. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Deferred by maintainer intake policy — not ignored. This current head ( A maintainer must explicitly take this PR or add a local frontend-review exception before it can receive substantive review. The defer label is a routing marker only, not a verdict on the change. |
|
Maintainer note: approved for review |
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head b8a46f6: the helper is the correct contract seam and its regression tests discriminate both source paths.
Fixes #7971.
Complements #7974, which fixed the desktop hover preview at its own call site (
aspect-[488/680]on the frame,h-full w-fullon the image). The four sites below are untouched by that commit and still letterbox, because they are the ones that let the image supply its own height.What
getCardImageSrcSetPropshands every card imagesizes="auto"plusloading="lazy", but nowidth/height. Without that pair the<img>has no intrinsic aspect ratio of its own: the spec's default object size (300x150) stands in, so an element that derives its height from the image lays out a 2:1 box, andobject-cover/object-containcrops or letterboxes the card.Measurements
Chromium 151, the shipped stylesheet, the site's own markup, only the attribute set differing:
w-[clamp(…)],object-cover)max-h-*/max-w-*,object-contain)w-fullinside anaspect-ratioparent,translateYsliceSites still affected on
main(fba2513)CardPreviewmobile compact peek (both branches)CardPreviewmobile modal, upright branchCardTextboxPrevieww-fullslice — thetranslateYoffset is a percentage of that height, so the visible band is wrong, not just the sizeCardCoverageDashboardThe other fifteen call sites of the helper constrain both axes in CSS and were never affected.
Why the helper rather than four more call sites
The helper is what introduces
sizes="auto", so it owes every consumer the intrinsic size that keyword resolves against; fixing call sites leaves the next one to rediscover this. It composes with #7974 rather than competing with it: CSS wins wherever a call site sizes the element, so the frame'saspect-[488/680]there stays authoritative and the pair only supplies the ratio nothing else provides.Both rungs are the same card scan at two widths, so pinning the
normalrung's 488x680 — the ratio the client's ownaspect-[488/680]placeholder tiles already encode — fixes the ratio for the whole ladder.Verification
cardImageSrcSet.test.ts: the two exact-shape assertions pinwidth/height, plus a contract test that no result may offersizes="auto"without them, for both the derived-URL and the installed-rung branch. jsdom has no layout, so this guards the contract, not the geometry — the geometry evidence is the table above.vitest run(full suite) 3648 passed,tsc -b --noEmitandeslintclean.