Prismic\Dom\RichText <img> width & height attributes

[husseinalhammad]

This PR adds the width and height attributes to the <img> tag returned by RichText::asHtml(). This prevents a layout shift when the image is downloaded and rendered, and is considered best practice.

[c0nst4ntin]

Can we be sure that dimensions and the properties width and height always exists?

If so, then everything is good. Otherwise we could use:

$element->dimensions?->width ?? ''

In case this get's changed we should ensure that there is also a testcase for this new scenario

[angeloashmore]

The dimensions.width and dimensions.height properties should always exist in API responses. The null fallback is still a good idea just in case someone is working on a very old repository that might be running on an older version of the API.

[c0nst4ntin]

Hey @husseinalhammad Thank you for your contribution to the package 👍🏼
This is a really good idea. I just have two small concerns:

1. I just had a look at your changes. They look good so far; I am just unsure if the properties you are trying to access exist in all cases. Do you have any information on this?

2. Furthermore, adding the width and height could cause people's websites to have an unexpected design change in case they changed the dimensions coming from prismic.

@angeloashmore How would you proceed with this? How is this handled in the Javascript Packages?

[husseinalhammad]

@c0nst4ntin

I just had a look at your changes. They look good so far; I am just unsure if the properties you are trying to access exist in all cases. Do you have any information on this?

Good point. Regardless of whether the Prismic API always returns these properties, one might want to use RichText::asHTML() to render data that is not coming from the Prismic API and they may not know what the dimensions are:

$html = RichText::asHtml([
    (object) [
        'type' => 'paragraph',
        'text' => 'Some text',
        'spans' => [],
    ],

    (object) [
        'type' => 'image',
        'url' => 'https://example.com/image.png',
    ]
]);

So I think it's a good idea to only add these tag attributes if the data is present.

Furthermore, adding the width and height could cause people's websites to have an unexpected design change in case they changed the dimensions coming from prismic.

If this change is added in a future release, the release notes can warn developers of this change. I don't see this as a blocker. Do you prefer to make this behaviour optional and not add the tag attributes by default?

[c0nst4ntin]

@husseinalhammad Thanks for your reply! Yes, I would definitely like for you to add the null checks on the dimension attributes. I'm not sure if "warning the developers" is enough and if this can be added in a minor release.

Changing the width and height of an image could severely break some designs.
Could you maybe add the null checks, while I try to get some feedback from @angeloashmore on this?

Furthermore, I fixed the CI in #207. Once this gets approved, the actions should also automatically test this PR.

[angeloashmore]

@c0nst4ntin The JavaScript SDK excludes the width and height attributes when using the default asHTML() serializer (see here). Image dimensions can be tricky to generalize and may be best suited for the consuming developer to define.

I don't know why we originally omitted width and height in the default HTML serializer. However, we'll likely continue to omit it to avoid breaking existing sites.

In this case, it might be safest to keep the serializer as it is today without the width and height attributes. The documentation could explain how to add both in using a custom HTML serializer.

As an aside, the React, Vue, Svelte, and Next.js SDKs have a dedicated <PrismicImage> component that computes the width, height, and srcset attributes. See @prismicio/react's source code for an example implementation: https://github.com/prismicio/prismic-react/blob/6d687fae0e26041e6648e44a9a68a904e47a5aa1/src/PrismicImage.tsx#L146-L183

[c0nst4ntin]

@angeloashmore So I take it, your comment is aligned with my concerns of breaking people's website designs by introducing this behavior? How do we proceed in this case? Do we go ahead and close this issue?

As far as I know, there is not a way to pass a custom serializer to the RichText Dom Class. Extending it is also not easily possible, as the methods are private instead of protected.

[angeloashmore]

@angeloashmore So I take it, your comment is aligned with my concerns of breaking people's website designs by introducing this behavior? How do we proceed in this case? Do we go ahead and close this issue?

As far as I know, there is now a way to pass a custom serializer to the RichText Dom Class.  Extending it is also not easily possible, as the methods are private instead of protected.

Correct, your concern is valid since this change could unexpectedly break projects.

I think either of the following options are reasonable:

1. Close this PR. If possible, update the asHTML method to accept an HTML serializer to allow for HTML attribute customization. It looks like the current method already accepts an htmlSerializer argument, but as you said, it might not be working.

2. Accept this PR and publish a new major version. Including the width and height attributes is technically more correct than excluding them.

Of the two, I lean more toward Option 2, but only if you think a breaking change won't cause unnecessary distress. With Prismic's official SDKs, we tend to be more conservative and avoid breaking changes as much as possible, offering simple workarounds or examples as needed (i.e. Option 1).

Ultimately, the decision is up to you, so go with whatever you are more comfortable with. 🙂

[c0nst4ntin]

@husseinalhammad Thank you very much for your efforts, but as discussed with @angeloashmore, this would be considered a breaking change. As of right now, I don't see this change alone justifying a major version. However, if I were to ever start working on a new version, this could definitely be implemented.

Have you tried using the custom serializer? Maybe that is a way for you to use your changes in your own project?

I will proceed to close this pull request for now. However, I still really value the time and effort you put into this, and I might revisit the topic at a later point in time. If you have any further troubles or want to suggest something different, feel free to open a new issue or pull request and discuss things with me and the team.

[husseinalhammad]

No problem at all. Thank you both for your time.

I have been using the custom serializer option before submitting this PR. It works as expected.