Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image rendering #3337

Merged
merged 62 commits into from
Jul 23, 2023
Merged

Image rendering #3337

merged 62 commits into from
Jul 23, 2023

Conversation

pnicolli
Copy link
Contributor

@pnicolli pnicolli commented May 6, 2022

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 plone/plone.restapi#1642

@netlify
Copy link

netlify bot commented May 6, 2022

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 17c3c28
🔍 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...

QR Code

Use your smartphone camera to open QR code link.

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

@pnicolli pnicolli requested a review from fredvd May 6, 2022 10:59
@cypress
Copy link

cypress bot commented May 6, 2022

Passing run #6541 ↗︎

0 529 20 0 Flakiness 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
Copy link
Contributor

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 #2103 i've added the inline-style with aspect-ratio value taked from real image sizes..
What do you think?

Copy link
Contributor Author

@pnicolli pnicolli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few implementation comments on point that imho we should discuss together @plone/volto-team

src/components/manage/Blocks/Listing/SummaryTemplate.jsx Outdated Show resolved Hide resolved
src/components/theme/Image/Image.jsx Outdated Show resolved Hide resolved
src/components/theme/Image/Image.jsx Show resolved Hide resolved
src/components/theme/Image/Image.jsx Outdated Show resolved Hide resolved
src/components/theme/Image/Image.jsx Outdated Show resolved Hide resolved
src/components/theme/Image/Image.jsx Outdated Show resolved Hide resolved
src/components/theme/Image/utils.js Outdated Show resolved Hide resolved
src/components/theme/PreviewImage/PreviewImage.jsx Outdated Show resolved Hide resolved
@pnicolli
Copy link
Contributor Author

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
Copy link
Member

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

@pnicolli
Copy link
Contributor Author

@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
Copy link
Member

sneridagh commented Jul 17, 2023

@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
Copy link
Member

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

@pnicolli
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

@davisagli @pnicolli 🟢

* master:
  feat(Url.js): add getFieldURL helper function to get the url value of a field based on its structure (#4731)
  Handle @linkintegrity response with items but no breaches (#4832)
@sneridagh
Copy link
Member

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

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tweak, then it's good to go. Thank you!

docs/source/upgrade-guide/index.md Outdated Show resolved Hide resolved
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@sneridagh sneridagh merged commit 262b755 into master Jul 23, 2023
39 checks passed
@sneridagh sneridagh deleted the image-rendering branch July 23, 2023 07:21
sneridagh added a commit that referenced this pull request Jul 24, 2023
* master: (42 commits)
  make selectedView and className props available for Search block (#4997)
  Release @plone/volto-testing 4.0.0-alpha.0
  Release 17.0.0-alpha.21
  Upgrade to Cypress 12.17.1 (latest) (#4981)
  Image rendering (#3337)
  feat(Url.js): add getFieldURL helper function to get the url value of a field based on its structure (#4731)
  Handle @linkintegrity response with items but no breaches (#4832)
  Release 17.0.0-alpha.20
  Use all the apiExpanders in use, so we perform a single request for getting all the required data. (#4946)
  Fix the condition deciding on listing pagination format so it takes into account container blocks as well (#4978)
  Release 17.0.0-alpha.19
  Fix search block input clear button doesn't reset the search (#4837)
  Add /ok route as an express middleware (#4432)
  handles condition for yearly frequency in recurrence (#4604)
  Remove dangling out of place Guillotina Cypress tests (#4980)
  Update to latest plone.restapi and Plone 6.0.6 (#4979)
  Update browserlist (#4977)
  `Links and references` view via content menu [Add `Links to item` view] (#4787)
  Release 17.0.0-alpha.18
  Toc responsive (#4912)
  ...
@tisto
Copy link
Member

tisto commented Sep 10, 2023

@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
Copy link
Member

I thought we had one, but not. I think there's a plain issue: #1428

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants