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 Notifications for Facility Cover Image Deletion #7609

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

AnkurPrabhu
Copy link
Contributor

Proposed Changes

@coronasafe/care-fe-code-reviewers @coronasafe/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

screenshot:
Screenshot 2024-04-15 at 12 57 27 AM

@AnkurPrabhu AnkurPrabhu requested a review from a team as a code owner April 14, 2024 20:07
Copy link

vercel bot commented Apr 14, 2024

@AnkurPrabhu is attempting to deploy a commit to the Open Healthcare Network Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

netlify bot commented Apr 14, 2024

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit ff121f1
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/6639c801ad7ae20008ebf282
😎 Deploy Preview https://deploy-preview-7609--care-egov-staging.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.

@AnkurPrabhu
Copy link
Contributor Author

@rithviknishad we already had notification for deletion its just that due to re-renders it does not show it. this should fix it

<CareIcon icon="l-save" className="text-lg" />
)}
<span>
{isDeleting ? `${t("Deleting")}...` : `${t("delete")}`}
Copy link
Member

Choose a reason for hiding this comment

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

why are we using capital key Deleting? 🤔

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 change it to deleting

Comment on lines 291 to 300
{isDeleting ? (
<CareIcon
icon="l-spinner"
className="animate-spin text-lg"
/>
) : (
<CareIcon icon="l-save" className="text-lg" />
)}
<span>
{isDeleting ? `${t("Deleting")}...` : `${t("delete")}`}
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have something simlar to disabling cover that disables the entrie action on save?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just used what we already do for upload we just wanted a re-render to happen here in order to fix the bug, should i remove it ?

@AnkurPrabhu
Copy link
Contributor Author

pls take a look @bodhish

@nihal467
Copy link
Member

LGTM

Copy link

github-actions bot commented May 2, 2024

Hi, This pr has been automatically marked as stale because it has not had any recent activity. It will be automatically closed if no further activity occurs for 7 more days. Thank you for your contributions.

@github-actions github-actions bot added the stale label May 2, 2024
@nihal467
Copy link
Member

nihal467 commented May 7, 2024

LGTM

Copy link
Member

@khavinshankar khavinshankar left a comment

Choose a reason for hiding this comment

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

@AnkurPrabhu can u also move the constants (that doesn't depend on any state) outside the function

@AnkurPrabhu
Copy link
Contributor Author

@AnkurPrabhu can u also move the constants (that doesn't depend on any state) outside the function

@khavinshankar can you explain as in what do you mean by constants here

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.

Add Notifications for Facility Cover Image Deletion
6 participants