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: add crop support to image services #12414

Merged
merged 7 commits into from
Nov 12, 2024
Merged

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Nov 11, 2024

Changes

Adds new fit and position parameter to ImageTransform, which tells an image service how to handle images when the requested aspect ratio is different fromt he source. Currently if both width and height are set then height is ignored. Now if fit is set then both are used, and cropping is enabled.

  • fit sets the behvior for an image service if the requested aspect ratio is different from the source image. Allowed values are CSS object-fit values, but it also accepts arbitrary strings that may be understood by a particular image service.
  • postion is used when cropping, to choose the point at which to focus the crop. It accepts CSS object-position values, but also allows arbitrary strings that may be supported by an image service. For example, the default sharp service support entropy for position, which focusses on the point with the most detail.

This PR implements this crop support in the default sharp image service. It also changes the behaviour when fit is set to set withoutEnlargement, ensuring the service never returns images that are larger than the source. The styling in the image component means that if the smaller image is returned then the upscaling will happen in the browser, which is more efficient.

Testing

The PR adds integration tests for the generation of srcsets. It also tests the actual image generation, ensuring the dimensions of the returned images are as expected.

Docs

Copy link

changeset-bot bot commented Nov 11, 2024

⚠️ No Changeset found

Latest commit: 41c2c7b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pr labels Nov 11, 2024
@ascorbic ascorbic changed the title wip: add crop support to image services feat: add crop support to image services Nov 12, 2024
@ascorbic ascorbic marked this pull request as ready for review November 12, 2024 12:39
@ascorbic ascorbic marked this pull request as draft November 12, 2024 13:20
@ascorbic ascorbic marked this pull request as ready for review November 12, 2024 14:31
@@ -180,6 +180,60 @@ describe('astro:image:layout', () => {
});
});

describe('generated URLs', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Github annotation says that logs (line 19) is unused

/** @type {import('./test-utils').DevServer} */
let devServer;
/** @type {Array<{ type: any, level: 'error', message: string; }>} */
let logs = [];
Copy link
Member

Choose a reason for hiding this comment

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

unused


describe('generated images', () => {
let $;
let src;
Copy link
Member

Choose a reason for hiding this comment

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

unused

@ascorbic ascorbic merged commit 46055a6 into responsive-images Nov 12, 2024
12 of 14 checks passed
@ascorbic ascorbic deleted the crop-image-service branch November 12, 2024 16:26
ascorbic added a commit that referenced this pull request Nov 15, 2024
* chore: changeset

* feat: add experimental responsive images config option (#12378)

* 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]>

* feat: add responsive image component (#12381)

* 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]>

* feat: add crop support to image services (#12414)

* wip: add crop support to image services

* Add tests

* Strip crop attributes

* Don't upscale

* Format

* Get build working properly

* Changes from review

* Fix jsdoc

* feat: add responsive support to picture component (#12423)

* 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

* docs: add docs for responsive images (#12429)

* docs: add responsive images flag docs

* docs: adds jsdoc for image components

* chore: updates from review

* Fix jsdoc

* Changes from review

* Add breakpoints option

* typo

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants