-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat: rename layout and refactor styles #13677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: d1bc212 The changes in this PR will be included in the next version bump. 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 |
CodSpeed Performance ReportMerging #13677 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changesets both look great! One tiny comment below from me! (And, this feels like a good change to me.)
.changeset/shaky-coins-fry.md
Outdated
|
|
||
| :warning: **BREAKING CHANGE FOR EXPERIMENTAL RESPONSIVE IMAGES ONLY** :warning: | ||
|
|
||
| The generated styles for image layouts are now simpler and easier to override. Previously it used CSS to set the size and aspect ratio of the images, but this is no longer needed. Now it just sets `object-fit` and `object-position`, and sets `max-width: 100%` for constrained images and `width: 100%` for full-width images. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great explanation of what's happening! My only nit is not being super clear on what "it" is referring to. (If it's "generated styles" then should be "they" for plural? If it's "layouts" that's also plural unless you just want to say "your layout used CSS". If it's Astro, then we haven't introduced that antecedent at all yet!)
Not a huge deal, but enough that I went, "wait a minute, what is "it" here?" so might be enough to be a bit distracting to readers.
Also, there's little guidance here as to what to do about this breaking change (if anything?). We could add something after this paragraph as another standalone paragraph so it stands out, like:
This is an implementation change only. However, it may affect any custom styles you have added to your responsive images. Please check your rendered images to determine whether any change to your CSS is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Reminder to update the RFC before shipping this new change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fantastic to me!
* feat: rename layout and refactor styles * Fix Vercel test * Fix jsdoc * Update wording
Changes
Renames experimental responsive image layout option from "responsive" to "constrained"
The previous name was causing confusion, because it is also the name of the feature. The
responsivelayout option is specifically for images that are displayed at the requested size, unless they do not fit the width of their container, at which point they would be scaled down to fit. They do not get scaled beyond the intrinsic size of the source image, or thewidthprop if provided.It became clear from user feedback that many people (understandably) thought that they needed to set
layouttoresponsiveif they wanted to use responsive images. They then struggled with overriding styles to make the image scale up for full-width hero images, for example, when they should have been usingfull-widthlayout. Renaming the layout toconstrainedshould make it clearer that this layout is for when you want to constrain the maximum size of the image, but allow it to scale-down.This also refactors and simplifies the generated styles.
Testing
Updated tests. Tested manually.
Docs
API docs updated. I will do a separate PR to update the experimental flag docs.