plone / plone.restapi

RESTful API for Plone.
http://plonerestapi.readthedocs.org/
84 stars 73 forks source link

Update image scales in blocks data when serializing internal links #1642

Closed davisagli closed 1 year ago

davisagli commented 1 year ago

This adds image_scales while serializing blocks (in the same serialization transform that handles resolveuid URLs), in order to support rendering images with a srcset (https://github.com/plone/volto/pull/3337)

image_scales is added at the same level as the url or @id key. That way the only change to the existing data structure is adding a new key, which should be backwards compatible in most cases.

mister-roboto commented 1 year ago

@davisagli thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

netlify[bot] commented 1 year ago

Deploy Preview for plone-restapi canceled.

Name Link
Latest commit 5b47b1f0b88fa5c9e17a41df42c9b4eeaeca1545
Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/649a20131c79dd0008d3aeeb
davisagli commented 1 year ago

@pnicolli My worry about backwards compatibility is this: The resolveuid serializer will process any field with the name url, even in custom blocks. So we can break them if those blocks suddenly start getting objects instead of strings there, and aren't expecting it.

Options for avoiding that:

  1. Put the image_scales beside the url instead of inside it:
    {
    "url": "/path/to/image",
    "image_scales": { ... }
    }
  2. Put the image_scales inside a url object, but only for the image block (still not 100% backwards compatible, but at least the change is more constrained in scope)
davisagli commented 1 year ago

@pnicolli I updated this to keep the url as a string and put image_scales at the same level -- but I haven't had time to try it with volto yet.

pnicolli commented 1 year ago

@davisagli thanks! I will give it a try today.

pnicolli commented 1 year ago

I tried pulling this branch and running my frontend branch against this, created an image block, saved it and image data is sent correctly by the rest api, so everything looks good!

pnicolli commented 1 year ago

Also tried replacing the image in the Image object and I am getting the correct new scales 👍

davisagli commented 1 year ago

@jenkins-plone-org please run jobs

davisagli commented 1 year ago

@jenkins-plone-org please run jobs

pnicolli commented 1 year ago

Tested this again with https://github.com/plone/volto/pull/3337 and it works fine for me.

davisagli commented 1 year ago

@jenkins-plone-org please run jobs

davisagli commented 1 year ago

@tisto Okay with you if I go ahead and merge this? I tested it in the context of our big project and didn't see any regressions.

sneridagh commented 1 year ago

@davisagli @tisto If you are ok, I will merge and release this.