-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(web): new feature photo #9443
Conversation
fe30b08
to
f44a78e
Compare
Deploying immich with Cloudflare Pages
|
@@ -433,6 +436,7 @@ | |||
{#each showPeople as person, index (person.id)} | |||
<PeopleCard | |||
{person} | |||
{now} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this a props of the component that must be passed down from the parent component seems strange. Can we isolate this logic in the people-card
component itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried at first, but if you search for a person and then go back to the full list, you won't use the cached thumbnails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically is equivalent to not using a cache to begin with, which is not ideal. We just need to reload the one image if/when it changes on the parent page. If you make a fetch request for it in the other component, will that refresh the cache so it is updated by the time you navigate here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now
in:
+page.svelte |
people-card |
---|---|
2024-05-13.23-20-02.mp4 |
2024-05-13.23-19-39.mp4 |
It's hard to see in a video, but when thumbnails are not cached it's not smooth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically is equivalent to not using a cache to begin with, which is not ideal. We just need to reload the one image if/when it changes on the parent page. If you make a fetch request for it in the other component, will that refresh the cache so it is updated by the time you navigate here?
let me try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing, the only way to force the new feature photo to update is to change its thumbnail path (because the image source hasn't changed). So the solution requires a lot more changes (probably adding a new query parameter, parsing its value, updating thumbnailPath...?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would adding an updatedAt
parameter fix that issue? If the person has changed, it will have a different updatedAt
value and the cache will be invalidated. But if they haven't changed, it will be the same value and the thumbnail can stay cached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that fixes it
f44a78e
to
b7ed1bc
Compare
b7ed1bc
to
22634b5
Compare
033777a
to
cfb033e
Compare
2865d16
to
578794b
Compare
578794b
to
19b185f
Compare
fixes #9437.
Currently, if you edit the main photo and return to the /people page, the thumbnails displayed are the previous cached ones. This PR changes the behavior to always force the browser to fetch the latest thumbnails when the /people page is loaded