-
Notifications
You must be signed in to change notification settings - Fork 32
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-4219: Add ability to print patient prescriptions #125
(feat) O3-4219: Add ability to print patient prescriptions #125
Conversation
Thanks, @Omoshlawi. Was there guidance to introduce the action column considering that other actions are shown when the row is expanded? We need additional guidance by the UI/UX team. Please reach out to Paul. |
Ok, sure, thankz |
also adding @fiona855 for her thoughts on the design... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment, but defer to others for a deeper code review. Also, I agree we should have a design discussion about where to add this print action (it probably is better as a button under "Prescription Details" where the Dispense-Pause-Close buttons are.
But thanks so much for working on this @Omoshlawi , this is definitely a much-requested feature.
<StructuredListCell head>Patient Name Here</StructuredListCell> | ||
<StructuredListCell head>Facility name here</StructuredListCell> | ||
</StructuredListRow> | ||
</StructuredListHead> */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this commented out text.
Agreed @mogoodrich. In the original designs (which have changed), the print button was a link under the prescription details which makes sense to me. |
Thanks, @fiona855 and @mogoodrich for the additions. We'll probably need to wait for Paul's guidance on the best positioning but @fiona855 can guide further. |
selectable-printable-prescriptions.mp4Added ability to select prescriptions to include in the print out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Omoshlawi I added a few comments but overall looks good to me, though again I defer to others with more O3 experience.
I think the layout and functionality look great... what are your thoughts @fiona855 ?
<StructuredListCell head> | ||
<br /> | ||
<br /> | ||
<p className={styles.printoutTitle}>Prescription Instructions</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use t() to localize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, sure.thanks
{patient.display | ||
.match(/^([A-Za-z\s]+)\s\(/) | ||
?.at(1) | ||
?.toUpperCase()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should manipulate the patient display here... I hope we have some sort of existing utility formatting method we can use... @denniskigen @ibacher ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted it into a utility function
<p>{dosageInstruction?.additionalInstruction[0].text}</p> | ||
)} | ||
</StructuredListCell> | ||
</StructuredListRow> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just scanned this quickly, but during testing we should make sure that this properly displays all the fields we want it to display.
Agreed, looks really good @Omoshlawi ! |
Thanks @Omoshlawi. It looks nice! |
@Omoshlawi are we good on this one? @mogoodrich anything else that needs to be addressed? |
Sorry for the delay, a holiday last week in the US. No blockers on my end, though it would be good to have @ibacher @denniskigen or @chibongho take a look. |
<p className={styles.printoutTitle}>{t('prescriptionInstructions', 'Prescription Instructions')}</p> | ||
{patient && ( | ||
<p className={styles.faintText} style={{ textAlign: 'center' }}> | ||
{extractpatientName(patient.display)?.toUpperCase()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denniskigen @ibacher @chibongho do we have an existing utility method we could use here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but this extractpatientName()
method is definitely not it (this regex excludes any accent marks, any non-Latin scripts, which means it does not work in a language-agnostic way). A much better (and simpler) way of doing the exact same thing is this:
display.includes('(') ? display.split('(')[0] : display;
We should probably add something like this to the framework utilities, though.
I'm also not hugely a fan of having the call to toUpperCase()
here. A CSS text-transform would be much cleaner here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing, the mixing of both className
and style
on a single tag should not be done. Just add a second class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Omoshlawi Did you see the above comments?
</span>{' '} | ||
— {dosageInstruction?.route?.text} — {dosageInstruction?.timing?.code?.text}{' '} | ||
{dosageInstruction?.timing?.repeat?.duration | ||
? 'for ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any display text must be translatable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks
<span className={styles.faintText}>{t('datePrescribed', 'Date prescribed')}</span> | ||
{': '} <span>{formatDate(parseDate((request.request as any).authoredOn))}</span> | ||
</p> | ||
{(refillsAllowed || refillsAllowed === 0) && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a drug order placed without refills needs to have that prominently noted on the printed prescription.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would stick with the text "No Refills".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, renamed
Ping @ibacher @mogoodrich |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See existing comments
@Omoshlawi please address the comments and the conflicts. |
conflict resolved |
Thanks @Omoshlawi, merging this in since all comments have been addressed. |
Thanks, @denniskigen for the reviews. @Omoshlawi please address the comments and have this in. |
Done, thanks |
…ng logic into utility function
…escription that are not refillable
…ndlers funtions and wrapping them in useCallback for persisting avoiding reconstructions
7c8f337
to
f6cf58c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Omoshlawi! I've taken the liberty to tack on some useful miscellaneous tweaks. Hope that's fine with you. Also, could you please add a summary of your changes to the PR description for posterity?
Requirements
Summary
Screenshots
prescriptionprint.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-4219
Other