fix(core): size gallery images to their grid cell, not the viewport - #3103
Merged
Merged
Conversation
Gallery.astro gave provider images sizes="(min-width: Wpx) Wpx, 100vw" and left locally stored images on Astro's constrained default, the same estimate. Each image renders in one of columns cells (two at 640px and below), so browsers picked srcset candidates several times wider than the cell. sizes now comes from gallerySizes(columns), which mirrors the gallery CSS including its 1rem gap, and applies to both the provider img and the AstroImage path. A new sizes prop overrides it for galleries rendered in a narrower container. Closes emdash-cms#2930
🦋 Changeset detectedLatest commit: a101b69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
area/core
size/M
review/needs-review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Approach judgment
This is the right change, solving the right problem, in the right place. The bug is real: Gallery.astro was telling browsers each image fills the viewport, so multi-column galleries downloaded oversized srcset candidates. Moving the sizes computation into a shared gallerySizes(columns) helper in media/responsive.ts keeps the responsive-image logic centralized and testable, and the new sizes prop addresses the issue's second direction (galleries inside narrower containers). The implementation mirrors the CSS grid rules, handles invalid column counts safely, and applies the override to both the provider <img> and Astro Image paths.
What I checked
- Full diff and the changed files (
Gallery.astro,media/responsive.ts, the new repro test, and the updated unit tests). - Call sites and siblings:
Image.astro,provider-loader.ts, existingimage-render.render.test.ts, andfocal-point-render.render.test.tsto confirm the__emdashTestMediaProviderspattern and provider rendering expectations. - AGENTS.md conventions for comments, changesets, and test quality.
.changeset/README.mdagainst the submitted changeset.
Headline conclusion
The code is sound, the tests reproduce the bug and verify the fix behaviorally, and the changeset is useful. I found only one minor AGENTS.md convention item in the new test file.
Findings
packages/core/tests/repro/gallery-sizes.render.test.ts(suggestion): the header comment references issue#2930. Per AGENTS.md comment discipline, issue/PR references should not live in code comments — that context belongs in the commit message and PR description. Keep the explanatory prose about the bug; just drop the issue number.
Comment on lines
+1
to
+7
| /** | ||
| * The gallery's `sizes` must describe a grid cell, not the viewport: each | ||
| * image renders in one of `columns` cells (two at 640px and below), and a | ||
| * `100vw` estimate lets the browser pick a far larger srcset candidate than | ||
| * the cell needs (#2930). | ||
| */ | ||
| import { experimental_AstroContainer as AstroContainer } from "astro/container"; |
Contributor
There was a problem hiding this comment.
[suggestion] The test header comment references issue #2930. Per AGENTS.md comment discipline, issue/PR references should not live in code comments — they become stale narrative once the change merges; that context belongs in the commit message and PR description.
Keep the explanatory prose about what the test reproduces; just remove the issue number.
Suggested change
| /** | |
| * The gallery's `sizes` must describe a grid cell, not the viewport: each | |
| * image renders in one of `columns` cells (two at 640px and below), and a | |
| * `100vw` estimate lets the browser pick a far larger srcset candidate than | |
| * the cell needs (#2930). | |
| */ | |
| import { experimental_AstroContainer as AstroContainer } from "astro/container"; | |
| /** | |
| * The gallery's `sizes` must describe a grid cell, not the viewport: each | |
| * image renders in one of `columns` cells (two at 640px and below), and a | |
| * `100vw` estimate lets the browser pick a far larger srcset candidate than | |
| * the cell needs. | |
| */ |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
review/awaiting-author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Gallery.astrotold browsers each image fills the viewport. Provider images gotsizes="(min-width: Wpx) Wpx, 100vw", and locally stored images with dimensions go through<Image layout="constrained">, whose default is the same estimate. But the gallery lays images out inrepeat(var(--columns, 3), 1fr)with a1remgap, and two columns at 640px and below. So the browser sizes the slot at roughly three times the rendered cell and picks a correspondingly largesrcsetcandidate. The local path's srcset runs up to 3200w for a 1600px image, so on DPR 2 screens this means downloading far more than each cell shows.gallerySizes(columns)inmedia/responsive.tsbuilds the cell estimate from the gallery's own CSS:(max-width: 640px) calc((100vw - 1rem) / 2), calc((100vw - 2rem) / 3)for three columns. One column gives100vwabove the breakpoint. A missing, zero or non-integer count falls back to three, the countGallery.astroalready defaults to.Gallery.astrouses it for both the provider<img>and theAstroImagepath.sizesprop overrides it. This is the issue's second direction, for galleries rendered in a container narrower than the viewport, where a viewport-based estimate is still too large. Both directions from the issue are covered: an accurate default and a consumer override.Closes #2930
Type of change
Checklist
pnpm typecheckpasses: not run astsgo(not installed here).tsc --noEmit -p packages/corereports no errors.pnpm lintpasses:oxlinton the touched filespnpm testpasses (or targeted tests for my change)pnpm formathas been run:oxfmt(via stdin on the LF content) andprettierfor the.astrofile, no changessizesprop is the override the issue proposes alongside it.AI-generated code disclosure
Screenshots / test output
tests/repro/gallery-sizes.render.test.ts(new) rendersGallerythrough the Astro container, with a mock provider injected via__emdashTestMediaProviders:columns: 4givescalc((100vw - 3rem) / 4)data-astro-image="constrained") gets the same estimatesizesprop winsWith
main'sGallery.astroandresponsive.ts:With this branch: 4 passed.
tests/unit/media/responsive.test.tsgainsgallerySizescases (column counts, one column, the fallback forundefined/0/2.5/NaN): 21 passed. The existingimage-renderandfocal-point-renderrepro suites still pass (23 tests across the three files).