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-3521: Add configurable ability to print multiple stickers on the same page #1934

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jnsereko
Copy link
Contributor

@jnsereko jnsereko commented Jul 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 the following configurability

  • specifying the size of the ID sticker on the printed page.
  • specifying the number of column and rows on each printing page.
  • specifying the number of ID stickers to be printed

This PR also adds ability to preview rows and columns before printing.
cc @denniskigen @vasharma05 @ibacher @pirupius

Screenshots

Video Preview

Screen.Recording.2024-10-24.at.14.35.07.mov

Related Issue

https://openmrs.atlassian.net/browse/O3-3521

Other

Screenshot 2024-10-24 at 14 39 24 Screenshot 2024-10-24 at 14 39 57 Screenshot 2024-10-24 at 14 40 17

Comment on lines 43 to 50
style={{
gridTemplateColumns: `repeat(${numberOfLabelColumns}, 1fr)`,
gridTemplateRows: `repeat(${numberOfLabelRowsPerPage}, auto)`,
}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denniskigen @ibacher I couldn't make this work via supplying a variable to in css and updating in here. Are you okay with this?

<div
key={index}
className={styles.printContainer}
style={{ height: printIdentifierStickerHeight, width: printIdentifierStickerWidth }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

<IdentifierSticker patient={patient} />
<span>
<Button kind="ghost" onClick={() => setIsPreviewVisible(!isPreviewVisible)}>
{!isPreviewVisible ? t('preview', 'Preview') : t('hidePreview', 'Hide Preview')}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denniskigen should i use t or getCoreTranslation here?

@pirupius pirupius changed the title Add configurable ability to print multiple stickers on the same page (feat) O3-3521 Add configurable ability to print multiple stickers on the same page Jul 23, 2024
@jnsereko jnsereko force-pushed the O3-3521 branch 2 times, most recently from 3fd3ae9 to 67b180a Compare July 28, 2024 19:12
package.json Outdated
@@ -19,7 +19,7 @@
"@hookform/resolvers": "^3.3.1",
"classnames": "^2.3.2",
"react-hook-form": "^7.46.2",
"react-to-print": "^2.14.13",
"react-to-print": "^3.0.0-beta-1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @ibacher

I didn't intend to do this migration since we might not be ready for it.
As you are aware, things are slightly different in beta v3, meaning i will have to refactor most of the printing logic in other parts of patient chart like vitals, medications, lab orders, etc.
I was testing if beta v3 actually solves the Type Error: can not read properties of undefined we experienced in #1847

Such migrations are the reasons why we might need a hook, wrapper or something like that to reduce the burden of writing alot of code when the react-to-print library hook parameters change
cc @brandones

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denniskigen, beta v3 solves solves the Type Error: can not read properties of undefined on the first printing attempt

Copy link
Member

Choose a reason for hiding this comment

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

Did you not notice that this version of the PR already does that refactoring?

@denniskigen denniskigen force-pushed the O3-3521 branch 2 times, most recently from a815733 to 245d57a Compare August 20, 2024 21:20
Comment on lines 79 to 82
printIdentifierStickerFields: Array<string>;
printIdentifierStickerSize: string;
printIdentifierStickerPaperSize: string;
numberOfPatientIdStickers: number;
numberOfPatientIdStickerRowsPerPage: number;
numberOfPatientIdStickerColumns: number;
useRelationshipNameLink: boolean;
printIdentifierStickerHeight: string;
printIdentifierStickerWidth: string;
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to sort these alphabetically.

Comment on lines 32 to 39
<style type="text/css" media="print">
{`
@page {
size: ${printIdentifierStickerPaperSize};
}
@media print { html, body { height: initial !important; overflow: initial !important; background-color: white; }}
`}
</style>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We have a copyStyles prop from the react-to-print lib, won't that suffice this?
Thanks!

@@ -0,0 +1,66 @@
import React, { forwardRef, useEffect, useImperativeHandle, useRef } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

First observation: Since we have so many files for print, and banner-tags have other tag components as well, please move all the print related files in a single folder

import styles from './print-identifier-sticker-content.scss';

interface PrintIdentifierStickerContentProps {
labels: Array<{}>;
Copy link
Member

Choose a reason for hiding this comment

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

Array<{} ?


interface PrintIdentifierStickerContentProps {
labels: Array<{}>;
numberOfLabelColumns: number;
Copy link
Member

Choose a reason for hiding this comment

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

We can make this columns and rows


const PrintIdentifierStickerContent = forwardRef<HTMLDivElement, PrintIdentifierStickerContentProps>(
({ labels, numberOfLabelColumns, numberOfLabelRowsPerPage, patient }) => {
const { printIdentifierStickerWidth, printIdentifierStickerHeight, printIdentifierStickerPaperSize } =
Copy link
Member

Choose a reason for hiding this comment

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

Since all these properties are related to a single workflow, you should club them all in a common object.

Comment on lines 26 to 48
if (numberOfLabelColumns < 1 || numberOfLabelRowsPerPage < 1 || labels.length < 1) {
return;
}
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 validated properly, that we always have at least 1 sticker to print, min value of the fields must be set to 1

Comment on lines 32 to 39
<style type="text/css" media="print">
{`
@page {
size: ${printIdentifierStickerPaperSize};
}
@media print { html, body { height: initial !important; overflow: initial !important; background-color: white; }}
`}
</style>
Copy link
Member

Choose a reason for hiding this comment

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

We have a copyStyles prop from the react-to-print lib, won't that suffice this?
Thanks!

Comment on lines 45 to 46
gridTemplateColumns: `repeat(${numberOfLabelColumns}, 1fr)`,
gridTemplateRows: `repeat(${numberOfLabelRowsPerPage}, auto)`,
Copy link
Member

Choose a reason for hiding this comment

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

What if the pagesize provided by the user is not able to accommodate the provided count, what will/should happen in such cases?
Thanks!

.labelsContainer {
display: grid;
column-gap: 1.3rem;
row-gap: 1rem;
Copy link
Member

Choose a reason for hiding this comment

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

layout.$spacing-05


.labelsContainer {
display: grid;
column-gap: 1.3rem;
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 1.25rem, given the styles in carbon are often used in multiples of .125rem
Thanks!

@ibacher
Copy link
Member

ibacher commented Aug 28, 2024

Why did we revert out the CSS property stuff? I also don't really understand the reversion to 2.x instead of 3.x...

@jnsereko
Copy link
Contributor Author

reversion to 2.x instead of 3.x...

@ibacher i have created a separate ticket to track the migration here since the API was changing and there are many other printing spots around the patient chart beyond the Identifier-Sticker

Why did we revert out the CSS property stuff?

Sorry @ibacher, I forgot to share here. Styles for some un-known reason were not being loaded into the print preview. For that reason, i was only able to print only a single page instead of 2 or more depending on the number of stickers per page and the number of rows and columns.
Also columns were wrapping in case 4 or more columns were specified.
@denniskigen shared that this is something he experienced some time back with the library so i created a ticket to debug that here

Supplying styles directly

Screen.Recording.2024-08-28.at.19.15.31.mov

Supplying styles indirectly using CSS file

Screen.Recording.2024-08-28.at.19.17.47.mov

@denniskigen denniskigen changed the title (feat) O3-3521 Add configurable ability to print multiple stickers on the same page (feat) O3-3521: Add configurable ability to print multiple stickers on the same page Sep 3, 2024
@denniskigen
Copy link
Member

denniskigen commented Oct 8, 2024

@jnsereko can we fix the conflicts now that #2022 is in.

@brandones
Copy link
Contributor

Ping @jnsereko , is it clear to you how to move forward?

@jnsereko
Copy link
Contributor Author

jnsereko commented Oct 23, 2024

I am fixing the tests. @vasharma05, could you be knowing why some fields on the Id sticker are hidden even on dev3?

@jnsereko
Copy link
Contributor Author

I am fixing the tests. @vasharma05, could you be knowing why some fields on the Id sticker are hidden even on dev3?

Found out that is occurs in case a patient has no address details defined.

@jnsereko
Copy link
Contributor Author

hey @denniskigen @ibacher @vasharma05, what do you think about this?

I am not so sure about the sticker size: height and width. I have defaulted it to auto so that it can automatically adjusted depending on the defined fields. This property can be helpful when arranging the number rows and columns across the to be printed page.

Since the sticker size is dynamic it will be up-to the user to determine the number of rows to print on each page without fields overflowing to the next page.

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.

5 participants