Overview
Currently, the image prop type consists of 4 properties: src, alt, width, and height.
This means a (Twig) SDC can render an image prop like this:
<img src="{{ image.src }}" alt="{{ image.alt }}" width="{{ image.width }}" height="{{ image.height }}" />
And a (JS) Code Component can render it like this:
export default function({ image }) {
return (
<img src={ image.src } alt={ image.alt } width={ image.width } height={ image.height } />
)
}
Or, taking advantage of JSX prop spreading, like this:
export default function({ image }) {
const { src, alt, width, height } = image;
return (
<img { ...{src, alt, width, height} } />
)
}
In #3515646: Add automated <img srcset> generation, we're working on adding a 5th property: srcsetCandidateTemplate. That name is still a work in progress. For the purpose of this issue, let's simplify it to scaledSrc. The idea is that src would be the original image, or possibly one with an image style applied for visual effect (color shifting, watermarking, cropping, etc.), but not for size optimization. Meanwhile, scaledSrc would be the URL template for scaling to one of several widths. For example:
src = '/sites/default/files/foo.jpg'
scaledSrc = '/sites/default/files/styles/xb_parameterized_width--{width}/public/foo.jpg.webp?itok=1Rl59WAb'
Assuming we add a toSrcSet filter, this would allow a Twig SDC to do this:
<img src="{{ image.src }}" alt="{{ image.alt }}" width="{{ image.width }}" height="{{ image.height }}" srcset="{{ image.scaledSrc|toSrcSet }}" sizes="auto 100vw" />
It would allow a JS code component to do something similar, but where JS components really shine is in the ability to use the JS ecosystem, such as next/image (or a version of it that can be used on its own without Next.js: next-image-standalone). So, for example:
import Image from "next-image-standalone";
export default function({ image }) {
const { src, alt, width, height, scaledSrc } = image;
const loader = ({ width }) => scaledSrc.replace('{width}', width);
return (
<Image { ...{src, alt, width, height, loader} } />
)
}
The above is nice and enables the code component to use various other nice features from next/image by passing the appropriate props. But one thing that's annoying about the above is the need to pass in an explicit loader prop. Although it's possible to configure a centralized loader, so long as it's relying on scaledSrc, the code component has to pass along scaledSrc somehow, just like the Twig version earlier.
I discussed this with @lauriii and he's been pushing back on this, asking: is it possible to let component authors just deal with standard <img> concepts like src, alt, width, and height, and not introduce any Drupalisms like a scaledSrc property?
Proposed resolution
What if instead of separate src and scaledSrc properties we combined both into src like this:
src = '/sites/default/files/foo.jpg?alternateWidths=%2Fsites%2Fdefault%2Ffiles%2Fstyles%2Fxb_parameterized_width--%7Bwidth%7D%2Fpublic%2Ffoo.jpg.webp%3Fitok%3D1Rl59WAb'
In other words, instead of a scaledSrc property we add it as a query parameter to src. This query parameter wouldn't affect the response if the browser requests this src (especially if foo.jpg is already on disk in which case query parameters do nothing), but it would allow the Twig SDC to do this:
<img src="{{ image.src }}" alt="{{ image.alt }}" width="{{ image.width }}" height="{{ image.height }}" srcset="{{ image.src|toSrcSet }}" sizes="auto 100vw" />
And more importantly, it would allow a JS code component to do this:
import Image from "next-image-standalone";
export default function({ image }) {
const { src, alt, width, height } = image;
return (
<Image { ...{src, alt, width, height} } />
)
}
Where a default loader function could be configured completely invisibly and from the perspective of the component author, they're using next/image completely idiomatically.
Issue fork experience_builder-3532718
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
lauriii💯 🤩 👏
Comment #3
balintbrewsWow. What an amazing idea! 🙇
Comment #4
effulgentsia commentedTagging 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.
Comment #5
libbna commentedI have given this issue a try: I checked out to the MR of #3515646 & 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.
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!
Comment #8
wim leersInitial review posted. Nice start!
Comment #9
isholgueras commented@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.srcwith the url to the image itself, without any style, and as a query parametersalternateWidthssend only the template (with the{width}and the itok) so the ui can react and modify.The decoded url you've written is
Is that exactly that we want to send in the
src=""And for
srcsetwhat we want to generate is a full list of allowed widths? Something like:I still don't get what is the HTML we want to return for this, sorry :(
Comment #10
wim leersNo, we'd want to strip
?alternateWidths=…Yes. Render the
sdc.experience_builder.imageSDC in HEAD and copy/paste the resulting markup. That is the end result you must achieve using these different mechanics 😊Comment #11
effulgentsia commentedAlthough we can choose to strip this from what the Twig renders into the
srcattribute if we want to, I don't think we 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 insrcsetanyway.This MR still needs, within
ShapeMatchingHooks.php, where we set the expression forsrcSetCandidateTemplateto instead change the expression ofsrcto be such that it evaluates to a URL that includes thealternateWidthsquery parameter. Eventually stuff like this could be done withAdaptedPropSourcebut 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?Comment #12
isholgueras commentedThanks both for your feedback! I know exactly what needs to be done.
Comment #13
wim leersThat's technically fine indeed 👍
Close! Making
srcbe different will require changing:\Drupal\experience_builder\Plugin\Field\FieldTypeOverride\ImageItemOverride::propertyDefinitions()\Drupal\experience_builder\TypedData\ImageDerivativeWithParametrizedWidth::getValue()\Drupal\experience_builder\Entity\ParametrizedImageStyle::buildUrlTemplate()They 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\ComponentInputsFormthat powers the tab support it today. Because it's blocked on design.Yep, my thoughts exactly!
On it 👍
Comment #14
wim leersExtra complexity here is that we only know at the
ImageItemlevel what the special sauce is, but thesrc(image URL) is currently coming from👆That follows the
imagefield type'sentityproperty, and on theFileentity that that points to, it instructs XB to retrieve theurifield'surlproperty. 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. 😅
Comment #15
wim leers— results for
ComponentInputsDependenciesTestThis 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 propertychain, 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!
Comment #16
wim leersGot 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
DynamicPropSources now won't work anymore: it continues to find what's in HEAD (follow theentityreference 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!Comment #17
wim leersComment #18
wim leersComment #19
balintbrewsI added all frontend pieces to make image props work in code components (
1ec1c952).You can test with the following code component:
Comment #21
effulgentsia commented@lauriii and I discussed that getting the image component DX right should actually block the beta, so I tagged #3535453: Create an Image SDC that can be included by other SDCs as such and promoting this one as well.
Comment #22
wim leersGot the shape matching pieces done.
With
0.6.0-alpha1out and working on this, my mind had the space to spot a whole range of concerns. Created #3536115: Allow use of same-shape-adapters ahead of general adapter support in #3464003 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,:
Once those are solved, I think this is ready to land! 🚀
Comment #23
balintbrewsI 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. 🙂
Comment #25
wim leersPushed commits adding
@todocomments pointing to #3464003: [PP-1] [later phase] [needs design] Introduce "adapters" UX, #3530351: Decouple image+video (URI) shape matching from specific image+video file types/extensions and #3536115: Allow use of same-shape-adapters ahead of general adapter support in #3464003.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
srcsetwidth 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 #3536115: Allow use of same-shape-adapters ahead of general adapter support in #3464003 tomorrow 🤓
Comment #27
wim leersThe only failure:
playwright, but that's a known pseudo-random fail: #3536108: The `playwright` CI job is also checking code style, but a failure is easily missed.Comment #28
neha_bawankar commentedTested changes on branch
0.x, for following scenarios :Comment #29
wim leersThanks!