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

(feat) O3-3165: Add modal confirmation modal when deleting repeated question #267

Merged
merged 12 commits into from
May 24, 2024

Conversation

NethmiRodrigo
Copy link
Contributor

@NethmiRodrigo NethmiRodrigo commented May 10, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR implements the functionality of triggering a confirmation modal whenever a user tries to delete a obsGroup repeated rendering question.
The form-engine-component has been updated to accept a new prop for the on click handler when deleting a question. Based on the prop, the component either calls the function passed in as the prop (which renders the modal) or proceeds with the deletion of the question (without showing a modal).

Screenshots

Screenshot 2024-05-24 at 6 35 13 PM

Related Issue

Jira Ticket
Related PR

Other

While this PR covers the intended functionality, it does bring forth some issues,

  1. The carbon version needed to be changed as it caused issues in the modal styling (namely the placement of the close icon, refer screenshot below).
Screenshot 2024-05-14 at 8 06 47 PM

@NethmiRodrigo NethmiRodrigo marked this pull request as draft May 10, 2024 13:48
@NethmiRodrigo NethmiRodrigo changed the title (feat): add modal for confirmation when deleting question (feat) O3-3165: add modal for confirmation when deleting repeated question May 10, 2024
@NethmiRodrigo NethmiRodrigo changed the title (feat) O3-3165: add modal for confirmation when deleting repeated question (feat) O3-3165: add modal confirmation modal when deleting repeated question May 10, 2024
Copy link

github-actions bot commented May 10, 2024

Size Change: -446 kB (-30.07%) 🎉

Total Size: 1.04 MB

Filename Size Change
dist/136.js 0 B -242 kB (removed) 🏆
dist/779.js 0 B -203 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size Change
dist/184.js 11.2 kB 0 B
dist/225.js 2.56 kB 0 B
dist/29.js 157 kB 0 B
dist/300.js 706 B 0 B
dist/31.js 5.28 kB +379 B (+7.74%) 🔍
dist/318.js 2.42 kB 0 B
dist/327.js 1.83 kB 0 B
dist/335.js 955 B 0 B
dist/353.js 3.02 kB 0 B
dist/371.js 71.3 kB -226 B (-0.32%)
dist/41.js 3.36 kB 0 B
dist/436.js 752 B 0 B
dist/465.js 181 kB 0 B
dist/540.js 2.64 kB 0 B
dist/55.js 2.13 kB -3 B (-0.14%)
dist/635.js 14.3 kB 0 B
dist/8.js 3.67 kB 0 B
dist/942.js 0 B -482 B (removed) 🏆
dist/979.js 6.85 kB 0 B
dist/99.js 690 B 0 B
dist/991.js 242 kB 0 B
dist/993.js 3.08 kB 0 B
dist/998.js 15.5 kB 0 B
dist/main.js 300 kB +117 B (+0.04%)
dist/openmrs-form-engine-lib.js 3.76 kB -4 B (-0.11%)

compressed-size-action

@samuelmale
Copy link
Member

The "delete" button of the modal should be coloured red.

@NethmiRodrigo
Copy link
Contributor Author

@samuelmale should we also show a snackbar/toast message after the user has pressed the confirm button on the modal?

@NethmiRodrigo NethmiRodrigo changed the title (feat) O3-3165: add modal confirmation modal when deleting repeated question (feat) O3-3165: Add modal confirmation modal when deleting repeated question May 17, 2024
@NethmiRodrigo NethmiRodrigo marked this pull request as ready for review May 23, 2024 11:23
@@ -69,6 +76,10 @@ export interface FormSubmissionHandler {
validate: (values) => boolean;
}

const functionStore = createGlobalStore<HandleConfirmDeletionFunctionStore | null>('functionStore', {
Copy link
Member

Choose a reason for hiding this comment

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

I think rather than using a global store for this, it makes more sense to use a React context. Context's are local to a React tree, so we have fewer issues around naming conflicts, etc. Global stores are for things we need to be shared across different React trees, but here I don't see a reason for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Ian. I've updated to using context instead, as suggested.
Could you please review the context part? I hope I've implemented it right.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM. I might rename the context ExternalFunctionContext or something so it's reusable for other similar functions (since I imagine we'll need something like this for other uses). Nice job!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. I changed the context name and pulled it out of the components/repeat nested folder into the root of src, because it looked wrong to have it nested there when made generic.

@ibacher
Copy link
Member

ibacher commented May 24, 2024

@samuelmale Any objections to merging this?

Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @NethmiRodrigo

@samuelmale samuelmale merged commit 89a3964 into main May 24, 2024
4 checks passed
@samuelmale samuelmale deleted the feat/deleteRepeatModal branch May 24, 2024 15:21
vasharma05 pushed a commit that referenced this pull request May 27, 2024
* (feat): add modal for confirmation when deleting question

* update test cases

* rename components to convention

* remove style sheet and update carbon version

* (fix): rename selectedQuestion state

* (fix): change primary button color of modal

* (refactor) Move modal to patient chart and have fallback for no modal

* (refactor): add confirmation modal handler as prop

* (fix): add error handler

* (fix): Replace global store with react context

* (refactor): Changed context name to be more generic

* Slightly cleaner @carbon/react version pinning

---------

Co-authored-by: Ian <ian.c.bacher@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants