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

Bust cover image cache after uploading a new image #8489

Conversation

Jacobjeevan
Copy link
Contributor

Proposed Changes

@ohcnetwork/care-fe-code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

@Jacobjeevan Jacobjeevan requested a review from a team as a code owner September 5, 2024 07:11
Copy link

netlify bot commented Sep 5, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit 872e353
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/66e993570094a2000853604e
😎 Deploy Preview https://deploy-preview-8489--care-ohc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Jacobjeevan Jacobjeevan marked this pull request as draft September 5, 2024 07:12
@Jacobjeevan
Copy link
Contributor Author

Converting back to draft as I just saw the comment about cache invalidation #8480 (comment)

@github-actions github-actions bot added the stale label Sep 14, 2024
- Refresh cover image on page, right after save
	- Previously required page reload for changes to show up on page
@Jacobjeevan Jacobjeevan marked this pull request as ready for review September 17, 2024 14:35
Comment on lines 189 to 191
facilityFetch();
fetchImage();
setCoverImageEdited(true);
Copy link
Member

Choose a reason for hiding this comment

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

Can we wait till the fetch image is done (in the loading state itself of the image edit modal) before running facility fetch?

Copy link
Contributor Author

@Jacobjeevan Jacobjeevan Sep 18, 2024

Choose a reason for hiding this comment

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

Sure, so something like this?

image

Copy link
Member

Choose a reason for hiding this comment

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

yes, but ensure the loading state is shown in the image edit modal till it completes.

src/Components/Facility/FacilityHome.tsx Show resolved Hide resolved
Comment on lines +61 to +62
const [coverImageLoaded, setCoverImageLoaded] = useState(false);
const [coverImageUrl, setCoverImageUrl] = useState<string | undefined>("");
Copy link
Member

Choose a reason for hiding this comment

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

And these unnecessay useStates too could be removed that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, coverImageLoaded is set based on the image load (addressing #8480 (comment)). Can remove the second one though.

Copy link
Member

Choose a reason for hiding this comment

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

but that state is being used only to decide whether the tooltip should be shown or not right? but when we are going to remove the tooltip anyhow, this state is not needed in the first place right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but that state is being used only to decide whether the tooltip should be shown or not right? but when we are going to remove the tooltip anyhow, this state is not needed in the first place right?

Yep, but are we removing the tool tip entirely? I thought it was to remove tool tip on successful image load.

yes, but ensure the loading state is shown in the image edit modal till it completes.

Hmm, well the modal closes immediately, so shouldn't be an issue. On a side note, we could use the tool tip here (and remove the coverImageLoaded state).

image

Comment on lines +89 to +90
imageUrl += "?" + Date.now();
setCoverImageUrl(imageUrl);
Copy link
Member

@rithviknishad rithviknishad Sep 17, 2024

Choose a reason for hiding this comment

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

Let's not add query params. We know for sure that it always fetches the latest image with query params present. There's no need to perform the vary header method when giving query params right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove 👍 I added query params since fetch/vary header behavior wasn't consistent on Firefox (though it's fine on Safari).

Comment on lines +86 to +87
credentials: "include",
mode: "no-cors",
Copy link
Member

Choose a reason for hiding this comment

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

are these headers required? how does this differ from with and without these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

credentials isn't required, mode is though; was getting 502 error for the fetch call without it.

@github-actions github-actions bot removed the stale label Sep 18, 2024
@sainak sainak changed the title Remove tooltip after Cover Image Load Bust cover image cache after uploading a new image Sep 19, 2024
@sainak
Copy link
Member

sainak commented Sep 21, 2024

with ohcnetwork/care#2472
we don't need to cleared the cache anymore

@Jacobjeevan Jacobjeevan deleted the issues/8480/tooltip-Removal-CoverImage branch October 9, 2024 05:41
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.

Remove the Tooltip Message After Cover Image Edit
4 participants