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

Remove the Tooltip Message After Cover Image Edit #8480

Closed
nihal467 opened this issue Sep 4, 2024 · 17 comments · Fixed by #8586
Closed

Remove the Tooltip Message After Cover Image Edit #8480

nihal467 opened this issue Sep 4, 2024 · 17 comments · Fixed by #8586

Comments

@nihal467
Copy link
Member

nihal467 commented Sep 4, 2024

Describe the bug

With ohcnetwork/care#2472 we no longer need to show the tooltip as it'll always show the new image.

To Reproduce
Steps to reproduce the behavior:

  1. Go to facility tab
  2. Click on any facility card
  3. upload a cover image to the facility
  4. See error

Expected behavior

  • Remove the tooltip message should

Screenshots

image

@nihal467 nihal467 changed the title Remove the fine text message post successful cover image loading Remove the Tooltip Message After Successful Cover Image Loading Sep 4, 2024
@Jacobjeevan
Copy link
Contributor

@nihal467 Can you assign this issue to me? Thx.

@rithviknishad
Copy link
Member

@Jacobjeevan

Let's clear the browser cache using vary header method mentioned in this https://www.fastly.com/blog/clearing-cache-browser. We can perform the cache invalidation once the cover image upload has been completed. This would ensure the latest image is rendered.

cc: @sainak

@Jacobjeevan
Copy link
Contributor

@rithviknishad Hmm, since useQuery doesn't support a header parameter, I'd either need to replace it with a fetch outside or modify RequestOptions interface and request function to support header property (preferring the former since it touches less code). Does that sound right, or is there an alternative option?

@rithviknishad
Copy link
Member

you'll have to use fetch directly to achieve this since the image url is not a defined api route right? useQuery/request requires an API route to be defined, and the image url is not an API route.

@rithviknishad
Copy link
Member

The additional headers on request options however is useful. Can be done in a separate issue/PR.

@Jacobjeevan
Copy link
Contributor

you'll have to use fetch directly to achieve this since the image url is not a defined api route right? useQuery/request requires an API route to be defined, and the image url is not an API route.

@rithviknishad Oh, I meant moving out the facilityData useQuery to a fetch req (since cover image url is part of that object). Hmm, as for fetching the image url directly, that would require modifications on the backend, right?

@rithviknishad
Copy link
Member

No, you just need to simply send a fetch request to the signed URL of the image with that vary header, so that browser would invalidate it's cache. Therefore leading to the image component to render the new image as the new image is fetched instead of the old cached one.

Nothing to do with calling any APIs.

@rithviknishad
Copy link
Member

@Jacobjeevan
Copy link
Contributor

No, you just need to simply send a fetch request to the signed URL of the image with that vary header, so that browser would invalidate it's cache. Therefore leading to the image component to render the new image as the new image is fetched instead of the old cached one.

Nothing to do with calling any APIs.

(1) Couldn't use the fetch + vary method as the HeadersInit type doesn't support it

Type '{ "Forced-Revalidate": number; }' is not assignable to type 'HeadersInit | undefined'.

(2) Tried fetch + cache:reload instead. Checking the network tab, it does pull the new version of the image, however I had to append timestamp to have it reflect on page (I tried changing image src directly using getElementById, but that didn't reflect on screen). fetchImage is called on save of the Edit Cover Image Modal/Popup.

It doesn't seem to update the cache though, since the change doesn't persist across page reload.

image

https://stackoverflow.com/questions/1077041/refresh-image-with-a-new-one-at-the-same-url/22429796#22429796 mentions few other solutions, including just using timestamp on img src directly, however that would mean storing multiple copies of the same image on cache.

@rithviknishad
Copy link
Member

I don't see any type errors in Force-Revalidate: random number as string method.

Numbers can't be set as header value. However, number as strings can be.

image

@Jacobjeevan
Copy link
Contributor

I don't see any type errors in Force-Revalidate: random number as string method.

Numbers can't be set as header value. However, number as strings can be.
image

Ah ok, thanks!

Wouldn't this require Vary: Forced-Revalidate on S3? Anyways, I tried with Forced-Revalidate: 1. But, I'm getting the same result as (2) in my reply above; new image is pulled by the fetch req in the network tab/dev tools, but only reflected on screen if I append a timestamp to the url. Cache doesn't seem to be updated as the change doesn't persist across page reload.

@rithviknishad
Copy link
Member

Did you try passing a key prop to the img. component?

@rithviknishad rithviknishad moved this from Up Next to In Progress in Care Sep 9, 2024
@Jacobjeevan
Copy link
Contributor

Did you try passing a key prop to the img. component?

Never mind, I cleared my browser cache/restarted the dev server and it's working (persists across page reload). Only other issue is the change not reflecting on FacilityCard (if the user navigates thru back button or breadcrumb menu instead of reloading).

@rithviknishad
Copy link
Member

Hey, could you push the current changes?

@Jacobjeevan
Copy link
Contributor

Hey, could you push the current changes?

Done 👍

@rithviknishad
Copy link
Member

#8489 (comment)

@rithviknishad rithviknishad closed this as not planned Won't fix, can't repro, duplicate, stale Sep 21, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Care Sep 21, 2024
@sainak sainak reopened this Sep 21, 2024
@sainak
Copy link
Member

sainak commented Sep 21, 2024

@rithviknishad we still need to remove

const [coverImageEdited, setCoverImageEdited] = useState(false);

{coverImageEdited && (
<div className="absolute inset-x-0 bottom-0 w-full rounded-b-md bg-black/70 px-2 pb-0.5 backdrop-blur-sm">
<span className="text-center text-xs font-medium text-secondary-100">
{t("cover_image_updated_note")}
</span>
</div>
)}

@rithviknishad rithviknishad changed the title Remove the Tooltip Message After Successful Cover Image Loading Remove the Tooltip Message After Cover Image Edit Sep 21, 2024
@rithviknishad rithviknishad moved this from Done to Up Next in Care Sep 21, 2024
@sainak sainak self-assigned this Sep 21, 2024
@sainak sainak moved this from Up Next to Review required in Care Sep 21, 2024
@github-project-automation github-project-automation bot moved this from Review required to Done in Care Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
5 participants