Improve the ContentMetadataTags component, add missing preview_image_link use case. Add exhaustive unit tests.
#5939
Conversation
…ge_link` use case. Add exhaustive unit tests.
✅ Deploy Preview for plone-components canceled.
|
✅ Deploy Preview for volto ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
||
| const getContentImageInfo = (image: PossibleImageFieldsUnion) => | ||
| match(image) | ||
| .with({ scales: { large: { download: P.string } } }, ({ scales }) => ({ |
There was a problem hiding this comment.
Note, an image can have scales but not have a large scale, if the full-size image is smaller than the large scale size.
There was a problem hiding this comment.
I forgot completely about it. If I keep forgetting about this, imagine others.
It's one of my biggest complaints in the last years. Can't we just make PIL generate them anyways with the highest resolution image possible? Having unpredictable API responses is not good.
There was a problem hiding this comment.
We're definitely not going to waste time and space generating and storing duplicate images.
In general, there's not much you can depend on about what image scales exist. The configuration can be changed per site in the registry. I think client code needs to be written to specify a target size (not a scale name), and use a function to pick the best scale for that size (i.e. either the smallest one largest than the target, or if there isn't one, then the largest one available)
There was a problem hiding this comment.
@davisagli doing that will require to know the scales info per site on every request (or at least, the first one). Should we do that?
There was a problem hiding this comment.
@sneridagh I don't think it requires any sitewide info, just the list of available scales for the specific image you want to use.
There was a problem hiding this comment.
Ok, so given the current info for the specific image you are handling, give me the nearest most appropriate one. Got it.
There was a problem hiding this comment.
I think that we should be indexing all scale information in the API, regardless they are bigger than the required scales. Right now we are doing one thing in the catalog results and another thing in the content serialization. See plone/plone.namedfile#140 for reference.
There was a problem hiding this comment.
That reminded me another one, why is the URL in some places context-less and in other there is the full URL? can we have just the full one everywhere (as in any other serialized URL in p.restapi)?
There was a problem hiding this comment.
@erral I discussed this with @sneridagh. We have to be careful about backwards compatibility but I think we can make some improvements.
- We need to store a URL without the hostname in the catalog, because it can be served on multiple hostnames. But, we can convert it to a full URL when serializing the scales for the API. It's technically not backwards compatible, but code that handles image scales usually has to handle both cases already, since it can be passed scales from the catalog or not.
- We can adjust the serializer for scales from the catalog to include all configured scale sizes (even if they are bigger than the original) but we should be careful to point to the same URLs as the original for those scales, to make sure we don't accidentally generate additional copies of the same image.
* main: (49 commits) Update to Plone 6.0.11 (#5989) Defines the last 4 parameters of the `asyncConnect` function with optional (#5986) Flexibilize the pins for all ESlint deps, in Volto and generators (#5991) Release 18.0.0-alpha.29 Release @plone/types 1.0.0-alpha.11 fix: pass down locale to IntlProvider (#5976) Add Vite (client only, no SSR) build. Update Next.js 14.2.2 and Remix to 2.8.0 (#5970) Fix no router link in logo (#5981) Improve postinstall script, building all the deps (#5980) Better BlocksData types (#5979) Add missing types ts fields Release 18.0.0-alpha.28 Release @plone/slate 18.0.0-alpha.12 Release @plone/registry 1.5.6 Improvements to the monorepo setup with utilities, especially ESLint.… (#5969) DEV: put nvm installation section into a separate include file (#5968) Bundle optimization (#5295) [client] Move provider to own package - Use parcel - `@plone/providers` (#5887) Fix flaky test 'As editor I can add links' by using getSlateEditorAndType (#5966) Fix rendering if ConditionalLink has no children (#5963) ...
@tiberiuichim finally I've found a a perfect use case to sneak
ts-patternin there :)