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.

Sign up for GitHub

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 experimental responsive images config option #12378

Merged
merged 5 commits into from
Nov 6, 2024
Merged

feat: add experimental responsive images config option #12378

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
91f84b0
feat: add experimental responsive images config option
ascorbic Nov 5, 2024
ff31c40
Apply suggestions from code review
ascorbic Nov 5, 2024
11e94b4
Update config types
ascorbic Nov 5, 2024
3babb85
Move config into `images`
ascorbic Nov 5, 2024
1e3d9fa
Move jsdocs
ascorbic Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions packages/astro/src/core/config/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export const ASTRO_CONFIG_DEFAULTS = {
experimental: {
clientPrerender: false,
contentIntellisense: false,
responsiveImages: false,
},
} satisfies AstroUserConfig & { server: { open: boolean } };

Expand Down Expand Up @@ -525,6 +526,22 @@ export const AstroConfigSchema = z.object({
.boolean()
.optional()
.default(ASTRO_CONFIG_DEFAULTS.experimental.contentIntellisense),
responsiveImages: z
.union([
z.boolean(),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder how updateConfig() will behave if the user sets it as a boolean, and an integration sets layout

z.object({
layout: z
.union([
Copy link
Member

Choose a reason for hiding this comment

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

you can use z.enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah duh!

z.literal('responsive'),
z.literal('fixed'),
z.literal('full-width'),
z.literal('none'),
])
.optional(),
}),
])
.optional()
.default(ASTRO_CONFIG_DEFAULTS.experimental.responsiveImages),
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
})
.strict(
`Invalid or outdated experimental feature.\nCheck for incorrect spelling or outdated Astro version.\nSee https://docs.astro.build/en/reference/configuration-reference/#experimental-flags for a list of all current experiments.`,
Expand Down
37 changes: 37 additions & 0 deletions packages/astro/src/types/public/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1699,9 +1699,46 @@ export interface ViteUserConfig extends OriginalViteUserConfig {
* To use this feature with the Astro VS Code extension, you must also enable the `astro.content-intellisense` option in your VS Code settings. For editors using the Astro language server directly, pass the `contentIntellisense: true` initialization parameter to enable this feature.
*/
contentIntellisense?: boolean;

/**
* @docs
* @name experimental.responsiveImages
* @type {boolean}
* @default `undefined`
* @version 5.0.0
* @description
*
* Enables and configures automatic responsive image generation for images in your project. Set to `true` or an object with configuration options.
* When enabled it will not transform any images unless you pass the `layout` option to the image component, or set the `layout` option in the `responsiveImages` configuration.
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
*
* ```js
* {
* experimental: {
* responsiveImages: {
* layout: 'responsive',
Copy link
Member

Choose a reason for hiding this comment

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

This now changes in that you only set this to true and now you go to regular image config to configure the properties, right?

And, since we now have multiple properties (acknowledging there may still even be more to come!), when you do add the regular image config for this example, I would set at least a couple of them.

(Not meaning to overly edit while stuff is still happening, just didn't want us to forget!)

* },
* }
* ```
*
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
*/

responsiveImages?:
Copy link
Member

Choose a reason for hiding this comment

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

should this be under image instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean as experimental.image, or at the top level?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the normal top level 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.

Is that something we do for experimental features?

Copy link
Member

Choose a reason for hiding this comment

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

We kinda have two philosophies, some people put it under experimental, some others don't. I personally think it makes the upgrading smoother when it's already where it will be, as you then don't need to move anything, but it's up to you really. It just felt a bit jarring to have both image and responsiveImages as two different options!

image in fact was top level while experimental, for instance

Copy link
Member

Choose a reason for hiding this comment

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

That works perfectly for me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarah11918 what are your thoughts on this? Should the config options for the feature go inside experimental.responsiveImages, or shoudl that just be a boolean, with the config sitting in the top-level image object? I'm conflicted!

Copy link
Member

@sarah11918 sarah11918 Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I saw this discussion and was happy to let it play out. 😅

We have precedence for both! experimental.env kept all its config within the experimental config and there was a time where astro:i18n had an option outside of the experimental config.

I don't have a strong opinion -- people are going to have to update their config either way in the cases you've described: either they move layout to the image option directly OR they update image.experimentalLayout to just layout. It sort of to me makes more sense in that case to keep all the config together?

The other option is also a bit wonky: just use image.layout BUT it only does anything with the experimental flag set. THIS is the option that means people don't have to update their config when it comes out of experimental (other than remove the flag). It's also not necessarily pretty to document something in the main config that is essentially no op without the flag, and, it means sending the user to two different places in order to configure the experimental feature. BUT it's the only thing that has the true advantage of needing less updating on behalf of the user.

So, I won't fight any of these options. I can make them all work and there's no clear and obvious winner here. I won't make the final call unless someone specifically wants me to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the absence of any strong opinions on either side, I'll leave it as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed my mind again! I've gone for the second option

| boolean
| {
/**
* @docs
* @name responsiveImages.layout
* @default `undefined`
* @description
* The default layout type for responsive images. Can be overridden by the `layout` prop on the image component.
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
*/
layout?: ResponsiveImageLayout | undefined;
};
};
}

export type ResponsiveImageLayout = 'responsive' | 'fixed' | 'full-width' | 'none';

/**
* Resolved Astro Config
*
Expand Down
Loading