Closed angeloashmore closed 2 years ago
💯 agree with rolling our own general-purpose imgix-ts
library. I'm alright for it also covering non-Prismic use cases and us investing time in it, but in a second phase maybe? (once it has proved a bit successful on @prismicio/helpers
and higher-level kits?)
For the proposed function names, are they part of imgix-ts
or @prismicio/helpers
?
If part of @prismicio/helpers
, I think I'd prefer that kind of API to better fit the current line up:
// Returns `src` attribute value
asImageSrc(imageField: ImageField, params?: ImgixURLParams)
=> string | null; // null in case of empty image field
// Returns `src`, `srcset`, and `sizes` attribute values
asImageSrcSet(imageField: ImageField, params?: ImgixURLParams & { sizes?: number[], ratios?: number[] })
=> { src: string; srcset: string, sizes: string | null } | null;
On
asImageSrcSet()
:I think it's simpler if it computes the desired matrix on its own according to provided parameters, avoiding the need for 2 helpers? Not sure we can mix
sizes
withratios
though, but we can have type exclusion if needed? Also, maybe it shouldn't return thesrc
attribute 🤔
If part of imgix-ts
I'm pretty fine with current names, maybe buildImageSrc
instead of transformImageURL
? Like you can add the transformation to all 3 helpers so I'm a bit bothered by the difference in naming of the API 🤔 I'd be also kinda for merging the two srcset helpers if possible.
<picture />
& <source />
considerationFair to mention them I think, I don't think we need <picture />
& <source />
solution 🤔 To me their main usage is for using the right image format which is automatically handled by imgix for us. What about them when you need 2 differents cropping depending on the display size though? Not sure.
Awesome work! Cheers~
For the proposed function names, are they part of
imgix-ts
or@prismicio/helpers
?
The proposed names are for @prismicio/helpers
. I like your names and revisions as they fit in with the other helpers nicely.
(I'll reference the old names for the rest of this comment, but I think we can explore using your proposed asImageSrc()
APIs)
On
asImageSrcSet()
:I think it's simpler if it computes the desired matrix on its own according to provided parameters, avoiding the need for 2 helpers? Not sure we can mix
sizes
withratios
though, but we can have type exclusion if needed?
Yep, we can use types to enforce only providing one of sizes
or ratios
.
I prefer the two separate functions as it makes it clear what each function does. They are also unable to get into an "undefined" state, such as providing both sizes
or ratios
. If we have one combined function and someone ignores the type error, or they are using JavaScript, we would need to prioritize one parameter over the other implicitly.
maybe
buildImageSrc
instead oftransformImageURL
?
This works for me!
Like you can add the transformation to all 3 helpers so I'm a bit bothered by the difference in naming of the API
The intention was that the srcset
helpers do not allow adding Imgix parameters (sorry, this wasn't clear in the examples). The options
parameter only allows options for building the srcset (e.g. sizes
). Applying Imgix URL parameters would first need to be done with transformImageURL()
.
const url = prismicH.transformImageURL(document.data.myImage.url, {
width: 400,
});
const srcset = prismicH.buildImageSrcSet(url, {
sizes: [400, 800, 1600],
});
This is done to avoid parameter conflicts with Imgix's URL parameters. If they decide to add a sizes
parameter, for example, it would conflict with our sizes
parameter.
<picture />
&<source />
consideration
I believe the URL and srcset helpers cover these cases. I don't think we should generate HTML, but the helpers could generate the srcset
attribute.
On
asImageSrcSet()
: I think it's simpler if it computes the desired matrix on its own according to provided parameters, avoiding the need for 2 helpers? Not sure we can mixsizes
withratios
though, but we can have type exclusion if needed?Yep, we can use types to enforce only providing one of
sizes
orratios
.I prefer the two separate functions as it makes it clear what each function does. They are also unable to get into an "undefined" state, such as providing both
sizes
orratios
. If we have one combined function and someone ignores the type error, or they are using JavaScript, we would need to prioritize one parameter over the other implicitly.
OK, works for me, asImageSrcSet()
and asImageDRPSrcSet()
then :)
Like you can add the transformation to all 3 helpers so I'm a bit bothered by the difference in naming of the API
The intention was that the
srcset
helpers do not allow adding Imgix parameters (sorry, this wasn't clear in the examples). Theoptions
parameter only allows options for building the srcset (e.g.sizes
). Applying Imgix URL parameters would first need to be done withtransformImageURL()
.const url = prismicH.transformImageURL(document.data.myImage.url, { width: 400, }); const srcset = prismicH.buildImageSrcSet(url, { sizes: [400, 800, 1600], });
This is done to avoid parameter conflicts with Imgix's URL parameters. If they decide to add a
sizes
parameter, for example, it would conflict with oursizes
parameter.
I don't really like having to chain two functions here (a bit similar to the scenario we had with the asLink(documentToLinkField())
helpers). Maybe for asImageDRPSrcSet()
we can use dpr
instead of ratios
to make it fit imgix API, and for asImageSrcSet()
maybe just assume that imgix won't add a sizes
attribute? (like I can see ratios
becoming one, as in, image ratio, but sizes
🤔) I think it's worth making a trade-off here, but might be wrong ^^'
<picture />
&<source />
considerationI believe the URL and srcset helpers cover these cases. I don't think we should generate HTML, but the helpers could generate the
srcset
attribute.
Yeah, definitely no HTML, just attribute values, but yes I don't have ideas about those yet and kinda think it's not necessary to cover them yet~
Thanks for the feedback!
I don't really like having to chain two functions here (a bit similar to the scenario we had with the asLink(documentToLinkField()) helpers). Maybe for asImageDRPSrcSet() we can use dpr instead of ratios to make it fit imgix API, and for asImageSrcSet() maybe just assume that imgix won't add a sizes attribute? (like I can see ratios becoming one, as in, image ratio, but sizes 🤔) I think it's worth making a trade-off here, but might be wrong ^^'
Makes sense to me! Calling the functions separately is a bit complex and requires you know how to use the two functions together. Okay, let's support URL params in the src set function like you described.
Re: ratios
vs dpr
I think it could be confusing to name the param dpr
or dprs
since it's similar to an existing Imgix param. We could exclude the dpr
param (since the function would override it anyway) and use a more descriptive property instead.
Options:
ratios
scales
(this option is straightforward to understand IMO)dprs
pixelDensities
const srcset = prismicH.asImageDPRSrcSet(url, {
sat: -100,
scales: [1, 2, 3],
});
Is the renaming the function to asImagePixelDensitySrcSet()
too verbose/confusing? Using an initialism (DPR
) might raise questions. For reference, the "pixel density" phrasing for srcsets is taken from here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Img#attr-srcset
const srcset = prismicH.asImagePixelDensitySrcSet(url, {
sat: -100,
pixelDensities: [1, 2, 3],
});
I'm pretty happy with pixel density (and the verbose-y name), shouldn't we follow the same logic for the first one? (or are we assuming width-based srcset are more common?)
asImageWidthSrcSet();
asImagePixelDensitySrcSet();
Also to fix that naming issue, I also thought about rolling the matrix as a 3rd parameter maybe (so it's safe for sure):
asImageWidthSrcSet(imageField: ImageField, params: ImgixURLParams | undefined, widths: number[]);
asImagePixelDensitySrcSet(imageField: ImageField, params: ImgixURLParams | undefined, pixelDensities: number[]);
Just makes it obvious and might help for implementation also?
asImageWidthSrcSet();
asImagePixelDensitySrcSet();
This makes sense to me. ✅
Also to fix that naming issue, I also thought about rolling the matrix as a 3rd parameter maybe (so it's safe for sure)
A third parameter can be awkward, in my opinion. Anything more than two arguments usually benefits from changing to a single object parameter.
In this case, splitting the params into two parameters means you must pass undefined
to params
if you don't want to apply image transformations. For example:
// Option #1
asImageWidthSrcSet(document.data.imageField, undefined, [400, 800, 1600]);
asImageWidthSrcSet(document.data.imageField, { sat: 100 }, [400, 800, 1600]);
// vs
// Option #2
asImageWidthSrcSet(document.data.imageField, { widths: [400, 800, 1600] });
asImageWidthSrcSet(document.data.imageField, { sat: 100, widths: [400, 800, 1600] });
Alternatively, the second parameter could be polymorphic. The function would have two signatures to support a widths
array or an object of URL params (which would then require a third param for widths
).
// Option #3
asImageWidthSrcSet(document.data.imageField, [400, 800, 1600]);
asImageWidthSrcSet(document.data.imageField, { sat: 100 }, [400, 800, 1600]);
I personally prefer Option 2, but Option 3 could also work well. The widths array is just less explicit since it isn't labeled with the widths
property.
Suggestion from @samlfair:
If you pass the field to
buildImageSrcSet()
, it could default to using the responsive views defined in Prismic.
This makes sense. If someone provides an explicit set of widths, then those take priority. Otherwise, the responsive views are used instead (if present).
This means we need to handle the case where we don't have any widths. This could happen if the field has no responsive views and an explicit set of widths was not given. We could have a fallback to a good set of widths (like [400, 800, 1600]
).
Options #4 would have been to switch parameters order (since the "matrix" is kinda mandatory with those helpers):
asImageWidthSrcSet(imageField: ImageField, widths: number[], params?: ImgixURLParams);
asImagePixelDensitySrcSet(imageField: ImageField, pixelDensities: number[], params?: ImgixURLParams);
Although, this doesn't play well with Sam's idea that I really like, so let's stick with options #2 (2 parameters)
:)
PR #38 contains a full implementation of this RFC
This is published in @prismicio/helpers
v2.1.0
🎉
Documentation for the new Image field helpers will be published on https://prismic.io/docs soon.
In the meantime, TSDocs can be viewed in the source files:
asImageSrc()
: https://github.com/prismicio/prismic-helpers/blob/master/src/asImageSrc.tsasImageWidthSrcSet()
: https://github.com/prismicio/prismic-helpers/blob/master/src/asImageWidthSrcSet.tsasImagePixelDensitySrcSet()
: https://github.com/prismicio/prismic-helpers/blob/master/src/asImagePixelDensitySrcSet.ts
Overview
This RFC addresses a need for straightforward image manipulation using Prismic’s Imgix integration.
What and/or who is affected by this RFC?
This affects all Prismic users using JavaScript in their projects with images.
The
@prismicio/helpers
library is also affected by this RFC.Libraries that use
@prismicio/helpers
, such as@prismicio/react
and@prismicio/vue
, may also be affected as a result of needing to provide new helpers, but are outside the scope of this RFC.Why is this being proposed?
Most users are not aware of Imgix’s capabilities and are not taking advantage of it (no data to back this up; just a hypothesis). We can enable users to make full use of Imgix’s features by providing an easy to use API.
Background Information
What is the current situation?
Today, all users have access to Imgix image transformations for all images uploaded to their Prismic repositories. Transformations are performed via URL parameters. For example, an image can be resized to 400px wide with the
w
parameter:The Prismic documentation currently recommends using manual string manipulation.
Source: https://prismic.io/docs/core-concepts/image#apply-your-own-transformations
Imgix provides a few libraries to make this process easier, such as
@imgix/js-core
andreact-imgix
. Rather than manipulating strings manually, a user can use these libraries to build URLs with a nicer API. For example:What are the current problems?
String manipulation is fragile and requires users to know exactly how the end URL should appear. This is a leaky abstraction; a user must understand Imgix to make use of image transformations. A user must also be careful to craft a correct and valid URL.
When working with a CMS where editors can change content, including image URLs and the URL parameters attached to them, building custom image URLs by hand can quickly become frustrating.
The API for Imgix’s core library is awkward to use since it requires instantiating a client and manually separating the URL’s domain (
testing.imgix.net
) from the image’s path (folder/image.jpg
). Prismic users should not have to care about this since images are always coming fromimages.prismic.io
.Imgix’s libraries are also relatively large and heavy since they need to support many different use cases. Prismic is typically used for public facing websites where library size is a strong consideration when deciding to integrate a feature. As a result, importing relatively large libraries for string manipulation is undesirable.
Are there any related discussions from elsewhere?
This proposal was sparked by a question from @samlfair. Repeated by @a-trost:
Proposal
Assume
@prismicio/helpers
is imported asprismicH
in the following code blocks.Applying image transformations should be as simple as a single function call to
transformImageURL()
:Removing parameters, such as
auto
(which is automatically applied to all Prismic images), should be straightforward as well:Building responsive images using
srcset
with<img>
could be generated with a function calledbuildImageSrcSet()
:Similarly, building a
srcset
using display pixel densities instead could be generated with a function calledbuildImageDPRSrcSet()
(DPR = device pixel ratio):As mentioned earlier, these helper functions could make their way into integration libraries like
@prismicio/react
and@prismicio/vue
. Such an API requires more thought and discussion and is outside the scope of this proposal.How it could be implemented
The core of the proposed functions is a simple URL manipulation function. It uses the native URL and URLSearchParams Web APIs to add, remove, and modify URL parameters.
Imgix provides a package named
imgix-url-params
that contains all valid URL parameters and their types. It comes in JSON format. With this, TypeScript types can be generated automatically with friendly comments. The parameters can be autocompleted with helpful links to Imgix’s documentation built in.The
srcset
functions would use this core function to build a string. Their implementation should be straightforward and is not included in this RFC.Supporting a general purpose Imgix library
The proposed helpers are not specific to Prismic. They are valid for any Imgix user. As such, we could treat these helpers as “general purpose” and make them available to anyone as a community open source project.
Such a library could support Imgix features that Prismic users would not use, such as building secure signed URLs and Web Proxy image sources. This could effectively replace Imgix’s official core library,
@imgix/js-core
, with a simpler, leaner API (at the expense of possibly not having 100% compatibility with Imgix).Similar unofficial libraries exist, such as Unsplash’s
ts-imgix
(https://github.com/unsplash/ts-imgix). None take the approach of completely replacing Imgix’s core library with near 100% feature support.This general purpose library is being developed as part of the exploratory phase of this RFC. For the purposes of this RFC, assume functions like
transformImageURL()
use this general purpose library in its implementation.How to provide feedback
Anyone familiar with Prismic’s image handling or wanting an easier image story can provide feedback.
If you have any thoughts, please reply to this issue.
Everything is open for discussion, but the following points are most important:
transformImageURL(url, params)
buildImageSrcSet(url, options)
buildImageDPRSrcSet(url, options)
Thanks! 🙂