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-2196: Implement the print functionality for Test results #1306

Merged
merged 19 commits into from
Aug 28, 2023

Conversation

nanfuka
Copy link
Contributor

@nanfuka nanfuka commented Aug 8, 2023

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

In case the "Show Print Button" is enabled in the configurations, the print button becomes visible within the header of the test results section. Upon clicking this print button, a popup emerges, housing a data table that showcases the patient's test results. This popup incorporates another print button. Activating this button initiates the printing process.

Screenshots

Screen Shot 2023-08-18 at 9 26 54 AM

The location of the print button

Screen Shot 2023-08-19 at 2 54 23 PM

The print modal when there are no test results for the selected period

Screen Shot 2023-08-15 at 10 00 13 AM

Appearance of print modal before any selections are made

Screen Shot 2023-08-15 at 11 15 18 AM

When a range of dates is selected

Related Issue

None

Other

None

Copy link
Contributor

@brandones brandones left a comment

Choose a reason for hiding this comment

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

Nice work. Please make the requested changes and we'll get it in.

@brandones
Copy link
Contributor

A few questions/requests:

  1. Why does the datepicker look so weird? It should look like this:
    image
    It should not have the drop-down overlaying the calendar. This seems obvious. Please don't commit things that have obvious problems without naming them explicitly. That introduces bugs, which wrecks the product.

  2. It's O3, not 03. That's the letter "O." The link to the issue was broken because you typed O3 as zero-three. O like OpenMRS.

  3. Your screen video doesn't show the actual Test Results dashboard with the new button on it. The button that you click to bring up the modal. Please show that so we can see what it looks like.

@brandones brandones changed the title (feat) Implement the print functionality for Test results (feat) O3-2196: Implement the print functionality for Test results Aug 8, 2023
packages/esm-patient-test-results-app/src/config-schema.ts Outdated Show resolved Hide resolved
},
{
"name": "print",
"component": "print"
Copy link
Member

Choose a reason for hiding this comment

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

Which slot does this extension get rendered into?

@Jexsie
Copy link
Contributor

Jexsie commented Aug 10, 2023

Thanks, @nanfuka. The Tablet also looks good

@nanfuka nanfuka marked this pull request as draft August 11, 2023 16:48
@brandones
Copy link
Contributor

Please provide an updated video once it looks right.

Comment on lines 37 to 34
const displayText = t('testReasults', 'Test results');
const headerTitle = t('testReasults', 'Test results');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const displayText = t('testReasults', 'Test results');
const headerTitle = t('testReasults', 'Test results');
const displayText = t('testResults', 'Test results');
const headerTitle = t('testResults', 'Test results');

@nanfuka
Copy link
Contributor Author

nanfuka commented Aug 14, 2023

Am yet to convert the date picker into the date range picker as suggested by the design team.

@brandones
Copy link
Contributor

Please @ me when ready for re-review, or hit the "request re-review" button if it is available to you

@nanfuka
Copy link
Contributor Author

nanfuka commented Aug 17, 2023

@brandones @denniskigen @hadijahkyampeire I've made the requested changes. Could you please re-review? Thank you!

@nanfuka nanfuka requested a review from brandones August 17, 2023 04:57
@brandones
Copy link
Contributor

Thanks for the updated images. Two notes:

  1. Taking inspiration from the designs,
    image
    I think that putting the print button to the left of the "Panel | Tree" switch would look better.

  2. I don't really understand "The print modal when there are no test results for the selected period." If there is a selected period, i.e. this happens after the user selects some dates, why would the datepicker go away? They should be able to select a different date range. On the other hand, if this comes up only when there are no test results at all (I don't know why you would write "for the selected period," then) it should still be styled like the regular print modal, with "Print test results" at the top. It looks right now like maybe you're just rendering the empty results tile as a card? Anyway it doesn't look right.

@brandones
Copy link
Contributor

Ok, just a few more suggestions.

@nanfuka nanfuka requested a review from brandones August 18, 2023 06:29
"Test Results": "Test Results",
"testName": "Test name",
"testReasults": "Test results",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"testReasults": "Test results",

When fixing typos, please use ctrl-shift-F global search to clean up. A global search of "testReasults" would have found this.

@brandones brandones marked this pull request as ready for review August 18, 2023 14:41
@brandones
Copy link
Contributor

image

@nanfuka nanfuka requested a review from brandones August 23, 2023 11:33
@nanfuka
Copy link
Contributor Author

nanfuka commented Aug 23, 2023

there is a CI build failure though. there is a failing test in esm common lib.

@brandones
Copy link
Contributor

I rebased your branch on main and pushed. That fixed the problem. Your branch was not up to date. You should keep your branches for your PRs up to date.

@brandones brandones merged commit c2ce7f6 into openmrs:main Aug 28, 2023
5 of 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