feat: add experimental responsive images config option #12378

ascorbic · 2024-11-05T11:08:12Z

Changes

Just creates the config option

Testing

Docs

changeset-bot · 2024-11-05T11:08:18Z

⚠️ No Changeset found

Latest commit: 1e3d9fa

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

Princesseuh · 2024-11-05T11:11:05Z

packages/astro/src/types/public/config.ts

+		 *
+		 */
+
+		responsiveImages?:


should this be under image instead?

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

Yeah, the normal top level image

Is that something we do for experimental features?

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

That works perfectly for me!

@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!

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.

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

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

sarah11918

SO EXCITED for this @ascorbic ! Just some super quick initial comments on the docs here, as they're getting worked out!

packages/astro/src/types/public/config.ts

florian-lefebvre · 2024-11-05T12:14:59Z

packages/astro/src/core/config/schema.ts

+					z.boolean(),
+					z.object({
+						layout: z
+							.union([


you can use z.enum

Oh yeah duh!

packages/astro/src/core/config/schema.ts

florian-lefebvre · 2024-11-05T12:16:07Z

packages/astro/src/core/config/schema.ts

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


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

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

packages/astro/src/types/public/config.ts

ascorbic · 2024-11-05T16:25:55Z

packages/astro/src/assets/types.ts

+	fit?: 'fill' | 'contain' | 'cover' | 'none' | 'scale-down' | (string & {});
+
+	position?: string;


I've gone with the shorter names for the HTML attributes for ease of use and avoidance of confusion of camel vs kebab

sarah11918 · 2024-11-05T20:16:42Z

packages/astro/src/types/public/config.ts

+		 * {
+		 *  experimental: {
+		 * 		responsiveImages: {
+		 * 			layout: 'responsive',


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!)

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

feat: add experimental responsive images config option

91f84b0

github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pr labels Nov 5, 2024

Princesseuh reviewed Nov 5, 2024

View reviewed changes

sarah11918 reviewed Nov 5, 2024

View reviewed changes

packages/astro/src/types/public/config.ts Outdated Show resolved Hide resolved

packages/astro/src/types/public/config.ts Show resolved Hide resolved

packages/astro/src/types/public/config.ts Outdated Show resolved Hide resolved

florian-lefebvre reviewed Nov 5, 2024

View reviewed changes

ascorbic and others added 2 commits November 5, 2024 15:52

Apply suggestions from code review

ff31c40

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

Update config types

11e94b4

ascorbic commented Nov 5, 2024

View reviewed changes

packages/astro/src/types/public/config.ts Outdated Show resolved Hide resolved

ascorbic commented Nov 5, 2024

View reviewed changes

ascorbic added 2 commits November 5, 2024 16:42

Move config into images

3babb85

Move jsdocs

1e3d9fa

ascorbic requested review from Princesseuh and florian-lefebvre November 5, 2024 16:54

sarah11918 reviewed Nov 5, 2024

View reviewed changes

florian-lefebvre approved these changes Nov 6, 2024

View reviewed changes

ascorbic merged commit 84492b0 into responsive-images Nov 6, 2024
12 of 14 checks passed

ascorbic deleted the resp-img-config branch November 6, 2024 06:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add experimental responsive images config option #12378

feat: add experimental responsive images config option #12378

ascorbic commented Nov 5, 2024

changeset-bot bot commented Nov 5, 2024 •

edited

Loading

Princesseuh Nov 5, 2024

ascorbic Nov 5, 2024

Princesseuh Nov 5, 2024

ascorbic Nov 5, 2024

Princesseuh Nov 5, 2024

Princesseuh Nov 5, 2024

ascorbic Nov 5, 2024

sarah11918 Nov 5, 2024 •

edited

Loading

ascorbic Nov 5, 2024

ascorbic Nov 5, 2024

sarah11918 left a comment

florian-lefebvre Nov 5, 2024

ascorbic Nov 5, 2024

florian-lefebvre Nov 5, 2024

ascorbic Nov 5, 2024

sarah11918 Nov 5, 2024

		fit?: 'fill' \| 'contain' \| 'cover' \| 'none' \| 'scale-down' \| (string & {});

		position?: string;

feat: add experimental responsive images config option #12378

feat: add experimental responsive images config option #12378

Conversation

ascorbic commented Nov 5, 2024

Changes

Testing

Docs

changeset-bot bot commented Nov 5, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarah11918 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Nov 5, 2024 •

edited

Loading

sarah11918 Nov 5, 2024 •

edited

Loading