Skip to content

(feat)O3-2971: Adding conformation modal for an empty form. #464

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

UNCANNY69
Copy link

@UNCANNY69 UNCANNY69 commented Feb 9, 2025

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 adds a feature which triggers a conformation modal when User submits an empty form.

Screenshots

OpenMRS.-.Google.Chrome.2025-02-11.23-09-36.mp4

Related Issue

Other

@UNCANNY69 UNCANNY69 changed the title Adding conformation modal for empty form (feat)O3-2971: Adding conformation modal for an empty form. Feb 9, 2025
@UNCANNY69
Copy link
Author

I made many changes to the form-factory-provider , I would like it to be reviewed in case it can be designed better @NethmiRodrigo @samuelmale @denniskigen

@UNCANNY69
Copy link
Author

Reated PR:

openmrs/openmrs-esm-patient-chart#2237

@UNCANNY69
Copy link
Author

I made the changes according to the discussion in the JIRA, can it be reviewed again @denniskigen @samuelmale @NethmiRodrigo

@UNCANNY69 UNCANNY69 requested a review from samuelmale February 12, 2025 16:52
@@ -127,6 +129,8 @@ const FormEngine = ({
provider={session?.currentProvider}
visit={visit}
handleConfirmQuestionDeletion={handleConfirmQuestionDeletion}
handleEmptyFormSubmission={handleEmptyFormSubmission}
handleDiscardForm={handleClose}
Copy link
Member

Choose a reason for hiding this comment

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

You may not have to add this considering that handleClose is basically a workspace property.

Copy link
Author

Choose a reason for hiding this comment

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

Then how should I approach this , should i call the workspace method directly?

Copy link
Member

Choose a reason for hiding this comment

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

After a second look, I realized you added this property to the FormFactoryProvider as opposed to the root component as I had previously assumed, my bad! It seems like we are lifting the handleClose property twice, any reason for not using formSubmissionProps.handleClose?

Copy link
Author

Choose a reason for hiding this comment

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

Cause that handleClose is passed as a void function in the formProps

Copy link
Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Ooppss!! That's something we should fix. Can you update those props with valid values from the parent component and use that instead? (It's probably safe to get rid of the onError prop)

Copy link
Author

Choose a reason for hiding this comment

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

There was an issue when i updated it with the value the form never opened ig that's the reason it was not passed as for removing onError properly i can do it.

Copy link
Member

Choose a reason for hiding this comment

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

Which error do you get exactly?

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.

4 participants