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

docs: add docs for responsive images #12429

Merged
merged 6 commits into from
Nov 15, 2024
Merged

Conversation

ascorbic
Copy link
Contributor

Changes

Updates the jsdocs for the experimental responsive images flag and config options, as well as the jsdocs for the Image component

Testing

Docs

This is a docs PR!

Copy link

changeset-bot bot commented Nov 14, 2024

⚠️ No Changeset found

Latest commit: 16a8c0a

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 14, 2024
@ascorbic ascorbic changed the title Resp img docs docs: add docs for responsive images Nov 14, 2024
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Thanks @ascorbic -- just left some small thoughts on this!

(The good news is that come v5, we will have individual, regular docs pages for experimental flags, so we don't need to get too caught up here yet. I'll be moving around and reformatting anyway, so we can just get the basics in here now and polish even more in the new format)

Comment on lines 1758 to 1764
* ```astro
* ---
* import { Image } from 'astro:assets';
* import { Image, Picture } from 'astro:assets';
* import myImage from '../assets/my_image.png';
* ---
* <Image src={myImage} alt="A description of my image." layout='fixed' />
* <Image src={myImage} alt="A description of my image." layout='responsive' width={800} height={600} />
* <Picture src={myImage} alt="A description of my image." layout='full-width' formats={['avif', 'webp', 'jpeg']} />
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be helpful to also show the rendered HTML below like we do for the basic <Image /> component. What do you think?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't is because it's very verbose, with a large srcset. I could do an abbreviated one though.

*
* ### Responsive image properties
*
* These are additional Image properties available when responsive images are enabled:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* These are additional Image properties available when responsive images are enabled:
* These are additional properties available to the `<Image />` component when responsive images are enabled:

Just confirming, not the <Picture /> component too? Only the <Image /> component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no, it's for both

* },
* }
* ```
*
* Then, you can add a `layout` option to any `<Image />` component when needed to override your default configuration: `responsive`, `fixed`, `full-width`, or `none`. This attribute is required to transform your images if `responsiveImages.layout` is not configured. Images with a layout value of `undefined` or `none` will not be transformed.
* When enabled, you can pass a `layout` props to any `<Image /> or `<Picture />` to enable automatic responsive images. You can also set a default value using `image.experimentalLayout`. When a layout is set, images have automatically generated `srcset` and `sizes` attributes based on the image's dimensions and the layout type. Images with `responsive` and `full-width` layouts will have styles applied to ensure they resize according to their container.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* When enabled, you can pass a `layout` props to any `<Image /> or `<Picture />` to enable automatic responsive images. You can also set a default value using `image.experimentalLayout`. When a layout is set, images have automatically generated `srcset` and `sizes` attributes based on the image's dimensions and the layout type. Images with `responsive` and `full-width` layouts will have styles applied to ensure they resize according to their container.
* When enabled, you can pass a `layout` prop to any `<Image /> or `<Picture />` to enable automatic responsive images on a per-image basis. Images will use the configured value of `image.experimentalLayout` by default, or the value passed to the component prop directly if provided.
*
* `srcset` and `sizes` attributes will be automatically generated based on the image's dimensions and the `layout` type. Images with `responsive` and `full-width` layouts will have appropriate styles applied to ensure they resize according to their container.

I wasn't sure whether setting image.experimentalLayout makes ALL your <Image /> components responsive by default, or (as my suggestion above is for), you still need to specify <Image layout /> but you just don't have to pass a value and the default set will be used.

So I think something like what I suggested above works for that second case. If the first case is actually true, then I think that could be made explicit more like:

Enabling this experimental flag allows you to configure image.experimentalLayout to set a default layout for all <Image /> components: responsive, fixed, full-width or none. You can override individual images by passing a layout prop with the desired value directly."

The main point of the feedback was that I wasn't entirely sure what the "default" behaviour is, so something that makes that more explicit is probably helpful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting experimentalLayout does enable it for all Images and Pictures. That's just a default though: you can instead enable it on an image-by-image basis, and if it is set, you can override it on each image too. I'll try to word that more clearly!

* - `position`: Defines the position of the image crop if the aspect ratio is changed. Values match those of CSS `object-position`. Defaults to `center`, or the value of `image.experimentalObjectPosition` if set.
* - `priority`: If set, eagerly loads the image. Otherwise images will be lazy-loaded. Use this for your largest above-the-fold image. Defaults to `false`.
*
* The `densities` prop is not supported when using responsive images. It is not recommended to use the `widths` or `sizes` props as these are automatically generated based on the image's dimensions and the layout type.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The `densities` prop is not supported when using responsive images. It is not recommended to use the `widths` or `sizes` props as these are automatically generated based on the image's dimensions and the layout type.
* The following `<Image />` component properties should not be used with responsive images as these are automatically generated and cannot be independently configured:
*
* - `densities`
* - `widths`
* - `sizes`

Maybe something like this? Make it more accurate (but doesn't have to be ENTIRELY and comprehensively accurate; just needs to say these are the ones not to use). I think framing them as the list not to use vs putting this info in paragraph text is more scannable.

@ascorbic ascorbic requested a review from sarah11918 November 15, 2024 11:11
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Amazing @ascorbic ! One tiny language preference suggested below for your consideration, but this is so exciting to get out and let people play with it! 🥳

* },
* }
* ```
*
* Then, you can add a `layout` option to any `<Image />` component when needed to override your default configuration: `responsive`, `fixed`, `full-width`, or `none`. This attribute is required to transform your images if `responsiveImages.layout` is not configured. Images with a layout value of `undefined` or `none` will not be transformed.
* When enabled, you can pass a `layout` props to any `<Image /> or `<Picture />` to enable automatic responsive images. When a layout is set, images have automatically generated `srcset` and `sizes` attributes based on the image's dimensions and the layout type. Images with `responsive` and `full-width` layouts will have styles applied to ensure they resize according to their container.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* When enabled, you can pass a `layout` props to any `<Image /> or `<Picture />` to enable automatic responsive images. When a layout is set, images have automatically generated `srcset` and `sizes` attributes based on the image's dimensions and the layout type. Images with `responsive` and `full-width` layouts will have styles applied to ensure they resize according to their container.
* When enabled, you can pass a `layout` props to any `<Image /> or `<Picture />` component to create a responsive image. When a layout is set, images have automatically generated `srcset` and `sizes` attributes based on the image's dimensions and the layout type. Images with `responsive` and `full-width` layouts will have styles applied to ensure they resize according to their container.

just tiny language thing (trying to stick to singular) and also, "automatic" feels a bit out of place here when this is manually defining how your image should be responsive. I think maybe saving "automatic" for the case where the image. experimental setting is configured further emphasizes that THAT is when stuff happens more "automatically" for you?

@ascorbic ascorbic merged commit a6b283a into responsive-images Nov 15, 2024
5 checks passed
@ascorbic ascorbic deleted the resp-img-docs branch November 15, 2024 12:14
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.

3 participants