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) Group Session Visits #37

Merged
merged 10 commits into from
Oct 5, 2022
Merged

(feat) Group Session Visits #37

merged 10 commits into from
Oct 5, 2022

Conversation

ZacButko
Copy link
Collaborator

@ZacButko ZacButko commented Oct 5, 2022

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.
  • My work conforms to the OpenMRS 3.0 Styleguide.
  • My work includes tests or is validated by existing tests.

Summary

This includes many changes but the most important change is that this is now saving a visit for a user when entering the Group Session data. After the visit is created the form data is posted with the new visitUUID attached. In then end when we check the patient's chart we can see their data now shows up on the visit.

Screenshots

GroupSessionVisits.mov

Related Issue

Other

@ZacButko ZacButko requested review from manuelroemer, vasharma05 and hadijahkyampeire and removed request for manuelroemer October 5, 2022 11:51
Copy link

@hadijahkyampeire hadijahkyampeire left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor suggestion, wouldn't it be better to make one ComposeModal Component that can be reused for the Cancel and Complete I see the content is almost the same.

Copy link
Member

@vasharma05 vasharma05 left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -53,7 +53,7 @@
},
"devDependencies": {
"@carbon/react": "^1.9.0",
"@openmrs/esm-framework": "next",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be kept for local purpose only?

Choose a reason for hiding this comment

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

I thought so as well but maybe we need to push the updated version if it won't break stuff, I think everyone needs to point to the right version of openmrs-framework. I will defer to Zac on what is best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

haha, yes, you are correct

@@ -259,6 +259,8 @@ const reducer = (state, action) => {
activeFormUuid: null,
};
persistData(newState);
//eslint-disable-next-line
Copy link
Member

Choose a reason for hiding this comment

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

Why using eslint-disable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The next line includes a template literal variable syntax in a non template literal string. This throws an eslint error, so the eslint-disable-next-line suppresses it.

@ZacButko
Copy link
Collaborator Author

ZacButko commented Oct 5, 2022

LGTM, just a minor suggestion, wouldn't it be better to make one ComposeModal Component that can be reused for the Cancel and Complete I see the content is almost the same.

The components are sufficiently different that it benefits having two different functions. One has two buttons, the other has three. Each modal calls very specific functions from the context it operates in, really if you were to try to abstract this you would be replicate most logic with two results, so in the end it is much more clear just to have two different components

@ZacButko ZacButko merged commit d38841c into main Oct 5, 2022
@ZacButko ZacButko deleted the feat/group-session-visits branch October 5, 2022 13:44
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