Image rendering

[pnicolli]

Related: #2103 #1428 #1395

~This is mainly documentation about how to properly render images, at this time. This is meant to be improved still, in order to document how we want to do this and then implement it.~

This now contains the new Image component implementation.

Depends on https://github.com/plone/plone.restapi/pull/1642

[netlify[bot]]

✅ Deploy Preview for volto ready!

Name	Link
🔨 Latest commit	17c3c28039db85c434fc2617ab387e4eba0888d7
🔍 Latest deploy log	https://app.netlify.com/sites/volto/deploys/64ba5ad0ef82490008857279
😎 Deploy Preview	https://deploy-preview-3337--volto.netlify.app/upgrade-guide/index.html
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[cypress[bot]]

Passing run #6541 ↗︎

0

529

20

0

0

Details:

Update docs/source/upgrade-guide/index.md
Project: Volto	Commit: 17c3c28039
Status: Passed	Duration: 16:48 💡
Started: Jul 21, 2023 10:19 AM	Ended: Jul 21, 2023 10:35 AM

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[giuliaghisini]

good. This morning i found out also that defining the aspect ratio in css could help the browser if you don't know the real size of the rendered image.. So, in https://github.com/plone/volto/pull/2103 i've added the inline-style with aspect-ratio value taked from real image sizes.. What do you think?

[pnicolli]

Note from Beethoven sprint discussion: in blocks that pick images (e.g. Hero and Image block), if we save image metadata like image_scales in the block data, we can end up having outdated information, because if the image changes, the scales change and the blocks data do not reflect that change.

Since the image selection is basically a url in the block data, and those are already being transformed by the resolveuid machinery, we could plug into that, check if the item is an image and update the block data accordingly, which likely also means deleting that data if the image is no longer there.

@davisagli feel free to correct me and/or add info.

[davisagli]

@pnicolli yes, that's right. I have code from a project that can be adapted for this.

[pnicolli]

Most of this is working. What is left is:

• styling the new reset button
• image upload is broken

Working on this in the next days

[pnicolli]

@sneridagh @davisagli should be quite fine now, but I am having trouble with cypress tests. The tests here on github most likely fail because the backend PR is missing, but I wanted to test cypress locally. Any suggestion on how to test a custom restapi branch with cypress locally? I found out how to configure volto and cypress but I don't know much about the backend and I need help configuring a custom backend or docker container to work for e2e tests.

[pnicolli]

I also still need to update the docs, will do asap.

[davisagli]

@pnicolli There's probably a better way but this seems to work:

1. Create a file named Dockerfile.acceptance containing:

   FROM plone/server-acceptance:latest
   RUN /app/bin/pip install git+https://github.com/plone/plone.restapi.git@block-image-scales#egg=plone.restapi

   (This extends the image built from https://github.com/plone/plone-backend/blob/6.0.x/Dockerfile.acceptance, which already includes the necessary changes for e2e tests like installing plone.app.robotframework and running on port 55001)

2. run docker build -f Dockerfile.acceptance -t volto-acceptance .

3. edit volto's Makefile to change DOCKER_IMAGE_ACCEPTANCE to volto-acceptance:latest

4. run make start-test-acceptance-server

[pnicolli]

@davisagli Thanks, seems like it's working, I am currently fixing tests.

[pnicolli]

All tests are now green on my machine with this restapi PR https://github.com/plone/plone.restapi/pull/1642

[pnicolli]

Also added docs now, I believe this PR is now ready.

[ksuess]

@pnicolli would you add to the PR description, that reviewing needs https://github.com/plone/plone.restapi/pull/1642?

[pnicolli]

Linting went crazy! Please see comments and revert.

@stevepiercy It is the other way around actually. All of those changes you saw there, it was me reverting my linter going crazy on the upgrade guide. In other words, I removed all the linting fixes that should not have been there because it's better if they go in a different PR. If you check the PR diff of all commits, you will see that the only change I am making to the upgrade guide in the master branch is this one:

[stevepiercy]

Linting went crazy! Please see comments and revert.

@stevepiercy It is the other way around actually.

OK, I was looking at GitHub's "View changes" view.

[pnicolli]

@sneridagh I don't understand this PR https://github.com/plone/volto/pull/4964/files I am inclined to resolve the conflicts by just removing all the changes done there for the teaser block DefaultBody component, because we don't need to handle a case where we don't have an Image component registered in the component registry. Am I missing something?

[sneridagh]

@pnicolli a part of that was fixing the use case of having a local override of the teaser image. If you are referring to amend the instantiation of the registered component using the "new one" used in the core component, then go ahead, by all means.

[sneridagh]

@pnicolli btw, master now have the latest p.restapi version (but I guess you already realised).

[pnicolli]

@pnicolli a part of that was fixing the use case of having a local override of the teaser image. If you are referring to amend the instantiation of the registered component using the "new one" used in the core component, then go ahead, by all means.

@sneridagh The current implementation in this branch does the same exact thing you did in that PR, exept this does not fallback to a native <img> tag. I would remove the fallback and all the code, basically restoring what you can see in this PR in that component, because we now have a component registered with the name Image by default in core volto.

[pnicolli]

I really don't understand why the unit tests are failing here.

https://github.com/plone/volto/actions/runs/5589278409

This is the error:

PASS src/components/manage/Blocks/LeadImage/Edit.test.jsx
PASS src/components/theme/Widgets/SelectWidget.test.js
PASS src/components/manage/Blocks/LeadImage/LeadImageSidebar.test.jsx
PASS src/components/manage/Blocks/Video/Edit.test.jsx
PASS src/components/manage/Blocks/Maps/View.test.jsx
PASS src/helpers/Api/Api.test.js
node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^

Error: connect ECONNREFUSED 127.0.0.1:8080
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1278:16) {
  errno: -111,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 8080,
  response: undefined
}

I don't get these errors on my local laptop, I actually get other errors, which are related to the Html SSR component and to auth token verifications - two things I did not touch at all anyway.

@plone/volto-team any pointers by any chance?

[sneridagh]

@davisagli @pnicolli 🟢

[sneridagh]

@stevepiercy I added some last minute docs regarding upgrade guide.

[tisto]

@sneridagh do we have a PLIP for this feature? Anything else that is left to do here? This is something we definitely want to mention when talking about Plone 6.1.

[sneridagh]

I thought we had one, but not. I think there's a plain issue: https://github.com/plone/volto/issues/1428