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

Show preview image in folder content on hover #6551

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

iRohitSingh
Copy link
Contributor

Fix #6550

Screen.Recording.2024-12-19.at.7.52.51.PM.mov

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 822d44f
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/67779550fcdba3000850ef14

@tisto
Copy link
Member

tisto commented Jan 3, 2025

@iRohitSingh the implementation looks good on the video. I have a few questions regarding the implementation:

  • did you test this for a folder contents view with 100 large images to see how the current implementation scales? I recall that @robgietema had put quite some effort into making the "image grid" option for the right content sidebar scale properly for our DLR project. Maybe he can point you to the implementation.
  • image on hover: we should lazy load those images or load the images only on hover to avoid long loading times for the folder contents view
  • image without hover: there was a PR some time ago from EEA (@avoinea, @tiberiuichim, @ichim-david?) for this feature. We did not enable or merge it because of loading performance concerns. We should try to look up that PR/discussion. In any case, if we load all images, we should at least use a smaller image size.

@@ -97,6 +103,8 @@ export const ContentsItemComponent = ({
order,
}) => {
const intl = useIntl();
const previewImageUrl = `${item['@id']}/${item.image_scales?.image?.[0]?.scales?.teaser?.download}`;
Copy link
Member

Choose a reason for hiding this comment

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

@iRohitSingh this lookup should be added in a new component that can be placed in this file
which is called if the item type is image. Right now you would attempt to check this for everything
even if there is no need for it.

title={item['Type'] || item['@type']}
/>{' '}
<span title={item.title}> {item.title}</span>
{item['@type'] === 'Image' ? (
Copy link
Member

Choose a reason for hiding this comment

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

All of @tisto advice is quite valid, lazy loaded when needed, you don't want to load 100 images if you never even interact with them.

Copy link
Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@iRohitSingh I left some concerns as comments in the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs discussion
Development

Successfully merging this pull request may close these issues.

Show Preview Image in folder contents view
4 participants