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

fix(ui): thread cache tag to list view thumbnails #11741

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

Conversation

akhrarovsaid
Copy link
Contributor

What?

This PR threads a cache tag to the Thumbnail component through the default File cell in the list view if cache tags are enabled in upload config. These changes also adjust the Thumbnail component to use useMemo instead of constructing a src later due to:

const img = new Image()
img.src = fileSrc

The above causes an extra request to be made if cache tags are enabled.

Why?

To thread cache tags through to the list view thumbnails.

How?

Changes the default File cell to pass the cache tag through if enabled, and changing a failing test to accommodate cache tags in the list view.

Addresses cache tag issue in #11690

* Check if the fileSrc already has a query string, if it does, append the imageCacheTag with an ampersand
*/
const queryChar = fileSrc?.includes('?') ? '&' : '?'
return imageCacheTag ? `${fileSrc}${queryChar}${encodeURIComponent(imageCacheTag)}` : fileSrc
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will encode false for no reason here? We should first check for the cache tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @JarrodMFlesch

Thanks for reviewing!

I think if imageCacheTag is false-y (or straight up false), then this code will fallback to fileSrc. If no fileSrc is provided, then it returns early with null unless I missed something here. Note the ternary operator in the return there. Am I overlooking a case?

Let me know!

* Check if the fileSrc already has a query string, if it does, append the imageCacheTag with an ampersand
*/
const queryChar = fileSrc?.includes('?') ? '&' : '?'
return imageCacheTag ? `${fileSrc}${queryChar}${encodeURIComponent(imageCacheTag)}` : fileSrc
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will encode false for no reason here? We should first check for the cache tag.

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.

2 participants