Image component for code components

This is great! I wonder if the following would be an improvement or not...

What if instead of a wrapper component, Image from "@/lib/Image", people used next-image-standalone directly, but we gave them a utility function for getting the loader function to pass in? For example:

import Image from "next-image-standalone";
import {useImageLoader} from "@/lib/utils";

const Cover = ({ image }) => {
  const loader = useImageLoader(image.srcSetCandidateTemplate);
  return (
    <div>
      <Image 
        src={image.src}
        alt={image.alt}
        width={image.width}
        height={image.height}
        loader={loader}
      />
    </div>
  );
};

export default Cover;

Would the above be more idiomatic in that it would be clearer that people are using essentially the stock Nextjs image component rather than "who knows what else @/lib/Image does"? Or is the @/lib/Image wrapper idea better because it gives us room to add other customizations, like adding some defaults to the config prop?

I prefer #5.1 over #5.2 because it's more similar to shadcn's open code approach. It makes the use of next/image (or the next-image-standalone version of it which is just a way to get next/image outside of a Next.js context) ultimately a decision made by the component author (even if we nudge that decision by providing an example), whereas to me it feels like #5.2 would imply that XB is making that decision more heavy-handedly.

I wonder if it could be misleading that we provide something called next-image-standalone, which may make it sound like a loader function is optional, just as it is with next/image

Is it reasonable for someone to expect "standalone" (by which is meant: usable outside of Next.js) to have a way of working without providing a loader function? If not, then I don't see anything misleading about it being required in standalone context, even if it's optional in non-standalone context.

Next.js allows configuring the default loader. I believe this is the most common way of configuring this – it doesn't make sense configuring this per instance unless there's something about a specific instance why it needs to use a different loader.

I think the ideal experience would we that we have a default loader which just works within the Drupal environment. There could be additional loaders for example to use CDN from a DAM to load an image, which would then potentially have to be loaded from within a component.

I don't think this goes against the open code approach because this is already an API that the upstream provides, which I also believe is the primary way of configuring this for Next.js applications.

it doesn't make sense configuring this per instance

But where to configure it then? There's no equivalent to next.config.js for code components unless we introduce some new convention for code component global JS config. We have a global CSS tab, so maybe adding a global JS tab makes sense?

I think the ideal experience would we that we have a default loader which just works within the Drupal environment.

We could do this per #5's answer about adding defaults to config. However, the default loader would still require a per-instance prop (srcSetCandidateTemplate) set that isn't part of the next/image set of props. So #4 could be simplified to:

import Image from "next-image-standalone";

const Cover = ({ image }) => {
  const {src, alt, width, height, srcSetCandidateTemplate} = image;
  return (
    <div>
      <Image {...{src, alt, width, height, srcSetCandidateTemplate}}/>
    </div>
  );
};

export default Cover;

However, I don't think <Image {...{src, alt, width, height, srcSetCandidateTemplate}}/> is any friendlier than <Image {...{src, alt, width, height, loader}}/>.

Some minor DX improvements we can make to #10...

1. srcSetCandidateTemplate is a mouthful. Perhaps this could just be srcTemplate.
2. Although in next/image the loader prop (when present) is always a function, perhaps next-image-standalone could allow it to be either a function or a string, and if it's a string, convert it to a function that replaces {src}, {width}, and {quality} in the string.

Doing both of the above would mean the #10 code would become:

import Image from "next-image-standalone";

const Cover = ({ image }) => {
  const {src, alt, width, height, srcTemplate} = image;
  return (
    <div>
      <Image {...{src, alt, width, height}} loader={srcTemplate} />
    </div>
  );
};

export default Cover;

But where to configure it then? There's no equivalent to next.config.js for code components unless we introduce some new convention for code component global JS config. We have a global CSS tab, so maybe adding a global JS tab makes sense?

Maybe we don't have to introduce a configuration at this point so long as we have set the correct default? I would imagine in Drupal this is something that a module may want to change, similar to what the CDN module is doing.

We could do this per #5's answer about adding defaults to config. However, the default loader would still require a per-instance prop (srcSetCandidateTemplate) set that isn't part of the next/image set of props.

Why do we need srcSetCandidateTemplate to be passed per instance? Why would I as the user of that component care about it? I would have no idea based on that prop name why it's there and why I'd have to pass it to the function. Tried to look at the code in the MR and in #3515646: Add automated <img srcset> generation but it still wasn't clear what it is.

@lauriii: I think you'll like #3532718: Improve the front-end DX of `<img srcset>`.

Proposal on that issue looks great! 👏

Tagging as beta blocker for the same reason as #3515646-51: Add automated <img srcset> generation.

Interesting discussion between @balintbrews & @effulgentsia #4 through #8 🤓

#10: We have a global CSS tab, so maybe adding a global JS tab makes sense? → note that our AssetLibrary infrastructure already supports this 😊

#11.1: srcSetCandidateTemplate is a mouthful. → it is named after the docs at https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/srcset..., specifically the "image candidate string" structure, follow-up for it at #3532718: Improve the front-end DX of `<img srcset>`.

Based on #13 through #15, I think this issue is actionable now that #3515646: Add automated <img srcset> generation landed (on Friday); the srcSetCandidateTemplate naming change is out of scope here; that's for #3532718: Improve the front-end DX of `<img srcset>`. AFAICT next steps here is for @balintbrews to respond to #10 and #11.

That would make it more complicated to do a new tab right now

Do you mean that it'd be better for this issue to wait for another MR (which one?) to add this tab for the Tailwind build? Or do you mean that the Tailwind build is [going to] just "piggybacking" on that infra and is storing JS in the global AssetLibrary, but won't be showing it in the UI?

👍

I tested this with the component in #21 against a fresh install with xb_vite running but after running npm run build

I get the following failure in the console and it doesn't render a preview.

I have run npm install as well

What am I missing?

[astro-island] Error hydrating /xb/api/v0/auto-saves/js/js_component/cover SyntaxError: The requested module 'http://127.0.0.1:8080/modules/experience_builder/ui/lib/astro-hydration/dist/jsxRuntime.module.js' doesn't provide an export named: 'jsx' hydration.js:1:3364
Uncaught SyntaxError: The requested module 'http://127.0.0.1:8080/modules/experience_builder/ui/lib/astro-hydration/dist/jsxRuntime.module.js' doesn't provide an export named: 'jsx' jsx-runtime-default.js:1:158

Code changes look good to me, just the issue with manual testing per #22

I didn't test this MR, but the code looks great to me! I'd consider it RTBC once #22 is addressed.

If you can't reproduce it, it might just be me doing something wrong

Paired with @balintbrews & @hooroomoo — turns out that we got the hardcoded widths in \Drupal\experience_builder\Routing\ParametrizedImageStyleConverter::ALLOWED_WIDTHS and components/image/image.twig (they must be manually set to be in sync until #3533563: Store allowed widths for xb_parametrized_width in third-party settings OR a separate simple config) wrong in #3515646: Add automated <img srcset> generation: we listed only the device sizes, we forgot the image sizes.

See https://nextjs.org/docs/pages/api-reference/components/image#imagesizes:

imageSizes allows you to specify a list of image widths. These widths are concatenated with the array of device sizes to form the full array of sizes used to generate image srcset.

Whoops!

Trivial fix here though :)

Tested changes on branch 0.x , following scenarios :

• Code Component named as “Image Component “
• Add Image prop and name it as “Image“
• Javascript as :
  

  const Image = ({ image }) => {
  return (<img {...image}  width={x} height={x} className="my-8 max-w-full"/>); };
  export default Image

Scenario	Result	Status
Edit “Image component” set widthxheight as 500x500 , On page Drag and drop “Image Component“ , add media (image of dimension 3000x2801)	View page , image dimension as 500x500	PASS
Edit “Image component” set widthxheight as 500x500 , On page Drag and drop “Image Component“ , add media (image of dimension 50x50)	View page , image dimension as 500x500 ,srcsetcandidatetemplate tag is present	PASS
Edit “Image component” set widthxheight as 50x50 , On page Drag and drop “Image Component“ , add media (image of dimension 3000x2801)	View page , image dimension as 50x50 ,srcsetcandidatetemplate tag is present	PASS
Edit “Image component” set widthxheight as 50x50 , On page Drag and drop “Image Component“ , add media (image of dimension 50x50)	View page , image dimension as 50x50 ,srcsetcandidatetemplate tag is present	PASS
Edit “Image component” remove widthxheight params , On page Drag and drop “Image Component“ , add media (image of dimension 3000x2801)	View page , image dimension as 3000x2801 ,srcsetcandidatetemplate tag is present	PASS
Edit “Image component” remove widthxheight params , On page Drag and drop “Image Component“ , add media (image of dimension 50x50)	View page , image dimension as 50x50 ,srcsetcandidatetemplate is present which should not be	PASS
Set image height and width in global css img { height: 50px; width: 50px; } Add media with dimension as (50x50)		PASS
Set image height and width in global css img { height: 50px; width: 50px; } Add media with dimension as (3000x2801)		PASS