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-2716: Prompt users to close workspaces when navigating from the patient chart #1607

Merged
merged 5 commits into from
Feb 2, 2024

Conversation

usamaidrsk
Copy link
Contributor

@usamaidrsk usamaidrsk commented Jan 22, 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 adds a prompt if navigating from the patient chart and the user has open workspaces, to ask if they wish to close the workspaces before navigating.

Screenshots

add_prompt-on-navigating-from-patient-chart-with-open-workspaces.mp4

Related Issue

O3-2716

Other

Copy link
Contributor

@njiddasalifu njiddasalifu left a comment

Choose a reason for hiding this comment

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

this looks good. Maybe a screenshot should be added

@vasharma05 vasharma05 changed the title feat: add prompt notification on navigating from patient chart when w… (feat) O3-2716: Prompt users to close workspaces when navigating from the patient chart Jan 24, 2024
@usamaidrsk usamaidrsk marked this pull request as ready for review January 24, 2024 09:41
Comment on lines +80 to +87
const navigateToNewUrl = () => {
function getUrlWithoutPrefix(url: string) {
return url.split(window['getOpenmrsSpaBase']())?.[1];
}
navigate({ to: `\${openmrsSpaBase}/${getUrlWithoutPrefix(newUrl)}` });
};

closeAllWorkspaces(navigateToNewUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not adequate?

Suggested change
const navigateToNewUrl = () => {
function getUrlWithoutPrefix(url: string) {
return url.split(window['getOpenmrsSpaBase']())?.[1];
}
navigate({ to: `\${openmrsSpaBase}/${getUrlWithoutPrefix(newUrl)}` });
};
closeAllWorkspaces(navigateToNewUrl);
closeAllWorkspaces(() => navigate({ to: newUrl }););

Copy link
Member

Choose a reason for hiding this comment

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

I referred this in the patient registration from Florian.

The newUrl contains the domain as well, hence it might not be the same handling by single-spa (navigating to /home vs navigating to dev3.openmrs.org/openmrs/spa/home). Navigate function calls the single-spa's navigation.


closeAllWorkspaces(navigateToNewUrl);
} else {
resetWorkspaceStore();
Copy link
Contributor

Choose a reason for hiding this comment

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

You should not be using resetWorkspaceStore, per the docstring for that function. What is your intention? Why should the workspace state be cleared?

Copy link
Member

Choose a reason for hiding this comment

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

Since the user is leaving the patient chart, the workspace can be reset to the initial state.

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!
Thanks!

@vasharma05 vasharma05 merged commit b9192c1 into openmrs:main Feb 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants