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: Correct relationship display on workspace forms #291

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

amosmachora
Copy link
Collaborator

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.
  • I checked for feature overlap with existing widgets.

Summary

In this PR I have refactored the **family-relationship other-relationship & case-management workspaces ** to use the correct display property on the UI while maintaining the integrity of the relationships themselves. Previously the when creating a relationship for example a sibling we were using the display property which caused the UI to have something like SIBLING/SIBLING. This PR correctly identifies the relationship direction of the subjects and finds out the correct display value to use improving the UX of the app.

Also I correctly handled the cyclic dependency for the relationships. For example right now if you create a relationship of Parent -> Child That child now has a parent . I guess that functionality was there but now the UI shows the correct information.

Screenshots

Before

image

After

image

Other Relationships Workspace (After)

image

Related Issue

None.

Other

None.

@ojwanganto
Copy link
Contributor

Thanks, @amosmachora. I defer to @donaldkibet and @makombe for their reviews.

donaldkibet
donaldkibet previously approved these changes Jul 29, 2024
Copy link
Contributor

@donaldkibet donaldkibet left a comment

Choose a reason for hiding this comment

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

LGTM Left some suggestions Thanks @amosmachora 🤝


data?.data.results.forEach((type) => {
const aIsToB = {
display: type.displayAIsToB ? type.displayAIsToB : type.displayBIsToA,
Copy link
Contributor

@ojwanganto ojwanganto Jul 30, 2024

Choose a reason for hiding this comment

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

What is exactly do we want to achieve with this? Should we switch the displays if one is missing? It may not be a good approach

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is how they have it implemented in the community. So I figured they probably had a good reason why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you think I should remove it ?

@amosmachora
Copy link
Collaborator Author

@donaldkibet are we good now. I refactored the hooks to avoid code duplication.

@ojwanganto
Copy link
Contributor

Please share the latest screenshot/video

@amosmachora
Copy link
Collaborator Author

Here is a video demonstrating the changes.

Relationships-display-demo.mp4

You can find the implementation code from the community here

@amosmachora
Copy link
Collaborator Author

@ojwanganto here is the video you requested.

Parent-child-relationship.mp4

ojwanganto
ojwanganto previously approved these changes Jul 31, 2024
Copy link
Contributor

@ojwanganto ojwanganto left a comment

Choose a reason for hiding this comment

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

Thanks, @amosmachora

@@ -49,12 +51,30 @@ export const useCaseManagers = () => {
return { data, error };
};

export const useRelationshipType = () => {
const customRepresentation = 'custom:(uuid,display)&q=Case manager';
export const useCaseManagerRelationshipType = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use the useRelationships hook and just filter the case manager relationships. What do you think?

@amosmachora
Copy link
Collaborator Author

Here is a screenshot to confirm the changes made have not broken anything.

image

and on case management.

image

@ojwanganto
Copy link
Contributor

Thanks, @amosmachora. This looks good

@ojwanganto
Copy link
Contributor

@donaldkibet please check this one more time so that we have it in

@ojwanganto ojwanganto merged commit 159019b into palladiumkenya:main Aug 1, 2024
7 checks passed
@amosmachora amosmachora deleted the relationships-display branch August 13, 2024 09:26
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