- Issue created by @effulgentsia
- ๐ณ๐ฑNetherlands balintbrews Amsterdam, NL
Wow. What an amazing idea! ๐
- ๐บ๐ธUnited States effulgentsia
Tagging this as beta target, because it would be nice to not have to either break BC after beta1 or support BC related to this. But not tagging it as a beta blocker, because it's not worth delaying a beta release on it.
- ๐ฎ๐ณIndia libbna New Delhi, India
I have given this issue a try: I checked the MR of #3515646 โจ Image component for code components Active & then I have updated the Image component to implement the proposed solution that eliminates the need for Drupal-specific props while maintaining full responsive image functionality.
import NextImage from 'next-image-standalone'; import type { ImageLoaderParams, ImageProps } from 'next-image-standalone'; const parseAlternateWidths = (src: string): string | null => { try { const url = new URL(src, window.location.origin); const alternateWidths = url.searchParams.get('alternateWidths'); return alternateWidths ? decodeURIComponent(alternateWidths) : null; } catch { return null; } }; export default function Image({ src, alt, width, height, ...props }: Omit<ImageProps, 'loader'>) { const loader = ({ width }: ImageLoaderParams) => { const alternateWidths = parseAlternateWidths(src); if (alternateWidths) { return alternateWidths.replace('{width}', width.toString()); } return src; }; return ( <NextImage src={src} alt={alt} width={width} height={height} loader={loader} {...props} /> ); }
- Removed srcSetCandidateTemplate prop
- Added URL parameter parsing via parseAlternateWidths
- Combined image source and responsive width info into single src prop
- Simplified component API for better DX (Developer Experience)
Before pushing the changes, Iโd appreciate it if someone could review the approach and let me know if any improvements or adjustments are needed. Thanks!
- First commit to issue fork.
- Merge request !1245Issue #3532718: Improve the front-end DX of <img srcset> โ (Merged) created by isholgueras
- ๐ช๐ธSpain isholgueras
@effulgentsia,
I'm missing something in your approach that is making me going back and forth in the ticket. I've pushed some code to help me explain that.
What you're proposing is to modify what is coming in the
img.src
with the url to the image itself, without any style, and as a query parametersalternateWidths
send only the template (with the{width}
and the itok) so the ui can react and modify.The decoded url you've written is
// /sites/default/files/foo.jpg?alternateWidths=/sites/default/files/styles/xb_parameterized_width--{width}/public/foo.jpg.webp?itok=1Rl59WAb
Is that exactly that we want to send in the
src=""
And for
srcset
what we want to generate is a full list of allowed widths? Something like:<img class="image" src="/sites/default/files/2025-07/2025-07-02_17-30.png" srcset="/sites/default/files/styles/xb_parameterized_width--640/public/2025-07/3534165-6_2.png 640w, /sites/default/files/styles/xb_parameterized_width--750/public/2025-07/3534165-6_2.png 750w, /sites/default/files/styles/xb_parameterized_width--828/public/2025-07/3534165-6_2.png 828w, /sites/default/files/styles/xb_parameterized_width--1080/public/2025-07/3534165-6_2.png 1080w, /sites/default/files/styles/xb_parameterized_width--1200/public/2025-07/3534165-6_2.png 1200w, /sites/default/files/styles/xb_parameterized_width--1920/public/2025-07/3534165-6_2.png 1920w, /sites/default/files/styles/xb_parameterized_width--2048/public/2025-07/3534165-6_2.png 2048w, /sites/default/files/styles/xb_parameterized_width--3840/public/2025-07/3534165-6_2.png 3840w" alt="Phandalin" width="1487" height="1003" sizes="auto 100vw" loading="lazy">
I still don't get what is the HTML we want to return for this, sorry :(
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Is that exactly that we want to send in the src=""
No, we'd want to strip
?alternateWidths=โฆ
And for srcset what we want to generate is a full list of allowed widths?
Yes. Render the
sdc.experience_builder.image
SDC in HEAD and copy/paste the resulting markup. That is the end result you must achieve using these different mechanics ๐ - ๐บ๐ธUnited States effulgentsia
No, we'd want to strip ?alternateWidths=โฆ
We can strip this from what the Twig renders into the
src
attribute, but we don't have to. I think it's okay for the Twig to just dosrc="{{ image.src }}"
as the MR already does and for that to mean the query parameter is retained. The query parameter is harmless other than adding some extra bytes to the HTML output, and those extra bytes are minor compared to what we end up needing insrcset
anyway.This MR still needs, within
ShapeMatchingHooks.php
, where we set the expression forsrcSetCandidateTemplate
to instead change the expression ofsrc
to be such that it evaluates to a URL that includes thealternateWidths
query parameter. Eventually stuff like this could be done withAdaptedPropSource
but I don't know if we have adapters working sufficiently well yet, so it might make sense to instead add another computed property toImageItemOverride
. XB's shape matching and prop expressions are kind of a tricky part of XB, so perhaps @wim leers could help out with this part? - ๐ช๐ธSpain isholgueras
Thanks both for your feedback! I know exactly what needs to be done.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
I think it's okay for the Twig to just do
src="{{ image.src }}"
as the MR already does and for that to mean the query parameter is retained.That's technically fine indeed ๐
This MR still needs, within
ShapeMatchingHooks.php
, where we set the expression forsrcSetCandidateTemplate
to instead change the expression ofsrc
to be such that it evaluates to a URL that includes thealternateWidths
query parameter.Close! Making
src
be different will require changing:\Drupal\experience_builder\Plugin\Field\FieldTypeOverride\ImageItemOverride::propertyDefinitions()
\Drupal\experience_builder\TypedData\ImageDerivativeWithParametrizedWidth::getValue()
\Drupal\experience_builder\Entity\ParametrizedImageStyle::buildUrlTemplate()
Eventually stuff like this could be done with
AdaptedPropSource
but I don't know if we have adapters working sufficiently well yetThey do on the back end at a low level; test coverage proves they work fine.
But โฆ neither the front-end (XB UI) nor the back-end's
\Drupal\experience_builder\Form\ComponentInputsForm
that powers the tab support it today. Because it's blocked on design., so it might make sense to instead add another computed property to
ImageItemOverride
. XB's shape matching and prop expressions are kind of a tricky part of XB, so perhaps @wim leers could help out with this part?Yep, my thoughts exactly!
On it ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Extra complexity here is that we only know at the
ImageItem
level what the special sauce is, but thesrc
(image URL) is currently coming from'src' => new ReferenceFieldTypePropExpression( new FieldTypePropExpression('image', 'entity'), new FieldPropExpression(BetterEntityDataDefinition::create('file'), 'uri', NULL, 'url'), ),
๐That follows the
image
field type'sentity
property, and on theFile
entity that that points to, it instructs XB to retrieve theuri
field'surl
property. String representation:srcโentityโโentity:fileโuriโโurl
.We'd now need to change that quite a bit, and crucially, in a way that the dependency information remains present. ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Drupal\Tests\experience_builder\Kernel\DataType\ComponentInputsDependenciesTest::testCalculateDependencies Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ 5 => 'file', 6 => 'node', 7 => 'file', - 8 => 'node', - 9 => 'file', ], 'config' => Array &2 [ 0 => 'node.type.alpha', @@ @@ 9 => 'node.type.alpha', 10 => 'field.field.node.alpha.field_hero', 11 => 'image.style.xb_parametrized_width', - 12 => 'node.type.alpha', - 13 => 'field.field.node.alpha.field_hero', - 14 => 'image.style.xb_parametrized_width', - ], - 'content' => Array &3 [ - 0 => 'file:file:d500fabe-6e85-4877-b250-a6d719fdb53e', ], ]
โ results for
ComponentInputsDependenciesTest
This is what I predicted in #14 about dependency information going missing: it's because we're no longer having XB itself follow the
image field โ entity reference โ file entity โ uri field โ url property
chain, hence XB doesn't know about this dependency: it's all abstracted away by this new computed field property.So that computed field property must somehow provide the correct dependency informationโฆ tricky!
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Got an initial solution for the dependency challenge above partially working. It needs more attention.
But due to the reliance on this computed property, we introduced a new challenge: the automatic (typed data/constraint-based) shape for finding candidate
DynamicPropSource
s now won't work anymore: it continues to find what's in HEAD (follow theentity
reference down to the file). So that logic will now also need to be updated for all tests to pass. If @isholgueras can get it to the point where that is the cause for the last remaining failures, that'd be great! - ๐ณ๐ฑNetherlands balintbrews Amsterdam, NL
I added all frontend pieces to make image props work in code components (
1ec1c952
).You can test with the following code component:
import Image from "next-image-standalone"; // Make sure you have an image prop named "Photo". const Cover = ({ photo }) => { return <Image {...photo} />; }; export default Cover;
- First commit to issue fork.
- ๐บ๐ธUnited States effulgentsia
@lauriii and I discussed that getting the image component DX right should actually block the beta, so I tagged ๐ Create an Image SDC that can be included by other SDCs Active as such and promoting this one as well.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Got the shape matching pieces done.
With
0.6.0-alpha1
out and working on this, my mind had the space to spot a whole range of concerns. Created ๐ Allow use of same-shape-adapters ahead of general adapter support in #3464003 Active for those. This issue doesn't make things worse (it follows an already established pattern), so we shouldn't block this on that.2 concerns about the public API of the new Twig function remain, @isholgueras is solving those,:
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
Once those are solved, I think this is ready to land! ๐
- ๐ณ๐ฑNetherlands balintbrews Amsterdam, NL
I made a small change in the starter code so we don't set a bad example with prop spreading. Thanks to @effulgentsia for pointing that out. I re-tested the code component functionality, still works great with the changes by this MR. The UI code probably could use a quick review as I wrote all of it, I wouldn't want to sign off on my own code. ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Pushed commits adding
@todo
comments pointing to โจ [later phase] PropShapes' JSON schema must match exactly with FieldType's storage format โ what if you want to use another FieldType? Active , ๐ Decouple image (URI) shape matching from specific image file types/extensions Active and ๐ Allow use of same-shape-adapters ahead of general adapter support in #3464003 Active .Addressed the last remaining concern.
Did a final clean-up pass to and in doing so, found an edge case bug: if the image itself was exactly the largest allowed parametrized image style width, we'd generate a
srcset
width for it, which makes no sense: it'd mean generating a derivative of identical dimensions as the original โ costing unnecessary CPU + storage resources to the server, and unnecessary network transfer for both client and server.Thanks @hooroomoo for the front-end approval!
Let's ship this ๐ โ and I'll see some of you in ๐ Allow use of same-shape-adapters ahead of general adapter support in #3464003 Active tomorrow ๐ค
-
wim leers โ
committed 003d4647 on 0.x authored by
isholgueras โ
Issue #3532718 by wim leers, isholgueras, balintbrews, justafish,...
-
wim leers โ
committed 003d4647 on 0.x authored by
isholgueras โ
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
The only failure:
playwright
, but that's a known pseudo-random fail: ๐ Some playwright jobs are failing with test:prettier error. Active .