feat(assets): Add support for srcset and a Picture component#8620
Merged
Princesseuh merged 18 commits intomainfrom Oct 11, 2023
Merged
feat(assets): Add support for srcset and a Picture component#8620Princesseuh merged 18 commits intomainfrom
srcset and a Picture component#8620Princesseuh merged 18 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 41f5fc6 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 |
github-actions
bot
added
the
pkg: astro
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Member
Author
|
!preview picture |
Contributor
|
Princesseuh
force-pushed
the
feat/picture-component
branch
from
63e341e to
7398d63
Compare
Member
Author
|
!preview picture |
Contributor
|
Member
Author
|
!preview picture |
matthewp
reviewed
Oct 6, 2023
matthewp
reviewed
Oct 6, 2023
matthewp
approved these changes
Oct 6, 2023
Contributor
|
!preview picture |
1 similar comment
Member
Author
|
!preview picture |
Member
Author
|
!preview picture |
bluwy
approved these changes
Oct 9, 2023
sarah11918
reviewed
Oct 11, 2023
Member
sarah11918
left a comment
sarah11918
left a comment
There was a problem hiding this comment.
Looking great @Princesseuh ! I made some small suggestions here, and I think a tiny bit more hand-holding description could make sense in one or two places, so see what you think!
zaknesler
reviewed
Oct 11, 2023
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
sarah11918
approved these changes
Oct 11, 2023
Member
sarah11918
left a comment
sarah11918
left a comment
There was a problem hiding this comment.
I love where the changset landed! I just saw one tiny missing "the" somewhere, but good to go!
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Merged
jasikpark
approved these changes
Oct 11, 2023
Contributor
jasikpark
left a comment
jasikpark
left a comment
There was a problem hiding this comment.
looks great! thanks so much for championing this!
| <picture> | ||
| <source srcset="..." type="image/avif" /> | ||
| <source srcset="..." type="image/webp" /> | ||
| <img src="..." alt="My super image in multiple formats!" /> |
Contributor
There was a problem hiding this comment.
will the img here be equivalent to the above image + densities example? a la "show the original file in the root image source?
|
|
||
| const image = await getImage(props); | ||
|
|
||
| const additionalAttributes: Record<string, any> = {}; |
Member
Author
There was a problem hiding this comment.
Mostly so we don't have to check the attributes before using them, since it doesn't really matter (it's classic Astro serializing, so it supports everything)
|
|
||
| const fallbackImage = await getImage({ | ||
| ...props, | ||
| format: fallbackFormat, |
Contributor
There was a problem hiding this comment.
ah, so the answer is yes!
| densities: props.densities, | ||
| }); | ||
|
|
||
| const additionalAttributes: Record<string, any> = {}; |
Contributor
There was a problem hiding this comment.
Suggested change
| const additionalAttributes: Record<string, any> = {}; | |
| const additionalAttributes: Record<string, unknown> = {}; |
perhaps?
| } | ||
| } | ||
|
|
||
| // TypeScript doesn't know this, but because of previous hooks we always know that targetWidth and targetHeight are defined |
Contributor
There was a problem hiding this comment.
why not have multiple return points, so typescript does know they're defined?
Member
Author
There was a problem hiding this comment.
Yeah, we could probably accept type params on image services, and have like a prop for validated props and stuff.
|
Thanks for your work! I was looking forward to it. I'll go and use it instead of my own little script, as soon as I get to it. |
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
8 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.
Changes
This implements the following RFC: withastro/roadmap#715. Please refer to it for more details on how the feature work.
Testing
Added tests
Docs
withastro/docs#4866