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

[Content] Improve image thumbnails #3224

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

finnar-bin
Copy link
Contributor

Show the appropriate thumbnail for non-image files
Resolves #3206

image

@finnar-bin finnar-bin added bug Something isn't working ready PR is complete and ready for deployment labels Feb 17, 2025
@finnar-bin finnar-bin self-assigned this Feb 17, 2025
};
const isFileZUID = !!params.value?.startsWith("3-");

const { data, isFetching } = useGetFileQuery(params.value, {
Copy link
Contributor

@agalin920 agalin920 Feb 18, 2025

Choose a reason for hiding this comment

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

Why? this will impact performance and this table is already very heavy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without fetching the media zuid's details, I can't get the need parameters to properly render the FileTypePreview. In this case, the filename, also I'm not sure if passing in the media-resolver as src would work fine with that component.

Copy link
Contributor

Choose a reason for hiding this comment

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

How is the filename used in this experience? Shown on hover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shrunyan it's used to determine whether or not the thumbnail is going to render an image or check what file type icon will be rendered.

Copy link
Contributor

Choose a reason for hiding this comment

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

@finnar-bin I see. Can we set up some time to run this locally and check performance dev tool tab to ensure we see no severe impacts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agalin920 yeah I agree. Sure, do you have any specific time in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I just realized, I could probably just do a find on all media assets instead of doing an api call to get this data, since we're already fetching all media on app load.

I'll update the PR first to make that change.

Copy link
Contributor Author

@finnar-bin finnar-bin Feb 28, 2025

Choose a reason for hiding this comment

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

@agalin920 I have updated it to use the cached data from useGetAllBinFilesQuery which is ran on app load. This way the api call is only done once and the ImageCell component only needs to do a .find to get the File data that it needs

@finnar-bin finnar-bin requested a review from agalin920 February 28, 2025 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready PR is complete and ready for deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content: Multi Page Table - File Type Column - Incorrect File Thumbnails
3 participants