Skip to content
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

feat: experimental responsive images #12377

Merged
merged 15 commits into from
Nov 15, 2024
Merged

feat: experimental responsive images #12377

merged 15 commits into from
Nov 15, 2024

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Nov 5, 2024

Changes

Implements responsive images RFC withastro/roadmap#1051

Testing

Docs

Copy link

changeset-bot bot commented Nov 5, 2024

🦋 Changeset detected

Latest commit: cc1db75

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

* feat: add experimental responsive images config option

* Apply suggestions from code review

Co-authored-by: Sarah Rainsberger <[email protected]>

* Update config types

* Move config into `images`

* Move jsdocs

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pr labels Nov 6, 2024
ascorbic and others added 2 commits November 8, 2024 13:58
* feat: add experimental responsive images config option

* Apply suggestions from code review

Co-authored-by: Sarah Rainsberger <[email protected]>

* Update config types

* Move config into `images`

* Move jsdocs

* wip: responsive image component

* Improve srcset logic

* Improve fixture

* Lock

* Update styling

* Fix style prop handling

* Update test (there's an extra style for images now)

* Safely access the src props

* Remove unused export

* Handle priority images

* Consolidate styles

* Update tests

* Comment

* Refactor srcset

* Avoid dupes of original image

* Calculate missing dimensions

* Bugfixes

* Add tests

* Fix test

* Correct order

* Lint

* Fix fspath

* Update test

* Fix test

* Conditional component per flag

* Fix class concatenation

* Remove logger

* Rename helper

* Add comments

* Format

* Fix markdoc tests

* Add test for style tag

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Nov 11, 2024
* wip: add crop support to image services

* Add tests

* Strip crop attributes

* Don't upscale

* Format

* Get build working properly

* Changes from review
* feat: add responsive support to picture component

* Add picture tests

* Fix test

* Implement own define vars

* Share logic

* Prevent extra astro-* class

* Use dataset scoping

* Revert unit test

* Revert "Fix test"

This reverts commit f9ec6e2.

* Changes from review
@ascorbic ascorbic added the pr: preview This PR has a preview release label Nov 14, 2024
@withastro withastro deleted a comment from github-actions bot Nov 14, 2024
@ascorbic ascorbic removed the pr: preview This PR has a preview release label Nov 14, 2024
* docs: add responsive images flag docs

* docs: adds jsdoc for image components

* chore: updates from review

* Fix jsdoc

* Changes from review
@ascorbic ascorbic changed the title wip: experimental responsive images feat: experimental responsive images Nov 15, 2024
@ascorbic ascorbic marked this pull request as ready for review November 15, 2024 12:16
width: resolvedOptions.width,
layout,
originalWidth,
breakpoints: isLocalService(service) ? LIMITED_RESOLUTIONS : DEFAULT_RESOLUTIONS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should people / image service be able to customise this? I'm notably talking about Vercel here, where the breakpoints needs to somewhat matches the allowed widths (hahaha)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did consider adding a breakpoints option to the config, though people can still set widths manually. Maybe it would be a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, config now supports image.experimentalBreakpoints

@@ -100,13 +158,23 @@ export async function getImage(
: [];

let imageURL = await service.getURL(validatedOptions, imageConfig);

const matchesOriginal = (transform: ImageTransform) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, couldn't the quality be different here? Or any other parameters that affects the image, for image services who support more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this scenario it can't, as this is just checking if it's the srcset entry which is the same as the src. I don't think we expose a way to set this separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't an image service set a different quality for a srcset transform?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in theory. I'm comfortable with not supporting that though. It's a terrible idea.

Comment on lines +25 to +27
750, // iPhone 6-8
828, // iPhone XR/11
1080, // iPhone 6-8 Plus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apple really is something

* - For fixed layout we return 1x and 2x the requested width, unless the original image is smaller than that.
* - For responsive layout we return all breakpoints smaller than 2x the requested width, unless the original image is smaller than that.
*/
export const getWidths = ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the comments in this one, great work.

getSrcSet(options) {
const srcSet: UnresolvedSrcSetValue[] = [];
const { targetWidth } = getTargetDimensions(options);
getSrcSet(options): Array<UnresolvedSrcSetValue> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't personally care all that much, but UnresolvedSrcSetValue[] is more common in our codebase

export { getConfiguredImageService, isLocalService } from "astro/assets";
import { getImage as getImageInternal } from "astro/assets";
export { default as Image } from "astro/components/Image.astro";
export { default as Picture } from "astro/components/Picture.astro";
export { inferRemoteSize } from "astro/assets/utils/inferRemoteSize.js";

export const imageConfig = ${JSON.stringify(settings.config.image)};
export const imageConfig = ${JSON.stringify({ ...settings.config.image, experimentalResponsiveImages: settings.config.experimental.responsiveImages })};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to add the new breakpoint option here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's part of settings.config.image.

@@ -284,6 +285,10 @@ export const AstroConfigSchema = z.object({
}),
)
.default([]),
experimentalLayout: z.enum(['responsive', 'fixed', 'full-width', 'none']).optional(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A single source of truth for this would be great, you can add it in const, derive the type from it and also use it here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cries in single source of truth for the config types ahem it would, yes

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a bunch of comments, but nothing blocking. Awesome work!

@ascorbic ascorbic merged commit af867f3 into next Nov 15, 2024
14 checks passed
@ascorbic ascorbic deleted the responsive-images branch November 15, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs pr pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants