Redesign print preview of drug chart#16446
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe PR refactors medication administration print UI by introducing fixed four-slot administration times (Morning, Afternoon, Night, Bedtime) replacing dynamic slot generation, adds checkbox-driven filtering for displayed slots, enriches the print header with location breadcrumbs and hospital ID, and redesigns the drug chart table with abbreviated slot headers and footer rows containing aggregated patient instructions and notes. ChangesMedication Administration Print UI Refactoring
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR redesigns the drug-chart print preview to match an updated Figma spec. The time-slot model moves from a dynamic generator (1–4 equal slots) to four fixed, named slots (Bedtime/Morning/Afternoon/Night), each user-selectable via a new checkbox panel. The header gains facility address, patient location breadcrumbs, and a hospital-ID field; each medication row gains a deduplicated instruction/note footer strip.
Confidence Score: 5/5Safe to merge — the change is scoped entirely to the print preview component and its locale strings, with no shared logic affected. The refactor is self-contained: the fixed ADMIN_TIME_SLOTS constant replaces the dynamic slot generator cleanly, adminIndex keying is consistent throughout, and the new helpers (locationPath, collectInstructionsAndNotes) are straightforward. The only concern is the HTML embedded in four locale strings, which is a future-maintenance risk rather than a current defect. public/locale/en.json — the four new keys with embedded HTML markup are worth revisiting if these strings ever need to be consumed outside a Trans context. Important Files Changed
Reviews (2): Last reviewed commit: "Merge branch 'develop' into ENG-521-rede..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <div className="mb-2 border border-gray-400 p-2 flex items-center justify-between"> | ||
| <span className="text-gray-950 text-xl font-semibold "> | ||
| {t("regular_medications")} | ||
| </span> | ||
| <div className="text-gray-600 text-base"> | ||
| <Trans | ||
| i18nKey="bed_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="morning_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="afternoon_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="night_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
The legend in the "Regular Medications" header hardcodes all four abbreviations (B, M, A, N) unconditionally. When a user deselects e.g. "Bedtime" and "Night" via the checkboxes, the table will only render "M" and "A" columns, but the legend still reads "B-Bedtime, M-Morning, A-Afternoon, N-Night" — making the printed chart misleading. The legend should be derived from timeSlots (the already-filtered list) rather than the full ADMIN_TIME_SLOTS constant.
| // Filter state | ||
| const [showDiscontinued, setShowDiscontinued] = useState(false); | ||
| const [slotsPerDay, setSlotsPerDay] = useState(4); | ||
| const [showDiscontinued] = useState(false); |
There was a problem hiding this comment.
showDiscontinued is always false — the setter was dropped when the toggle UI was removed. Since the value never changes, useState adds noise here; a plain constant is clearer and keeps the intent explicit.
| const [showDiscontinued] = useState(false); | |
| const showDiscontinued = false; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const slotLabels = timeSlots.map((slot) => slot.abbreviation); | ||
|
|
||
| // Thicker divider after the final slot of a day; zebra shade alternate slots. |
There was a problem hiding this comment.
slotLabels is built solely to be indexed by slotIdx inside the same timeSlots.map callback where slot.abbreviation is already in scope. The extra array is unnecessary indirection.
| const slotLabels = timeSlots.map((slot) => slot.abbreviation); | |
| // Thicker divider after the final slot of a day; zebra shade alternate slots. | |
| // Thicker divider after the final slot of a day; zebra shade alternate slots. |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx`:
- Around line 449-475: The legend currently always renders all four Trans labels
inside PrintMedicationAdministration; change it to render only the labels that
match the component's current slot filter state/prop (e.g.,
selectedSlots/slotFilters) so the legend stays in sync with the table columns.
Locate the legend block in PrintMedicationAdministration and wrap each Trans
(i18nKey="bed_time"/"morning_time"/"afternoon_time"/"night_time") in a
conditional that checks the slot is included in the active filter array
(preserve ordering and comma separators when multiple are shown). Ensure you use
the existing slot filter state/prop used by the table (the same variable that
determines which columns render) so both legend and columns stay consistent.
- Around line 327-333: The checkbox onCheckedChange currently allows removing
the last selected slot which leads to timeSlots.length === 0 and colSpan being
set to 0; change the handler that updates selectedSlotKeys (the onCheckedChange
using setSelectedSlotKeys) to guard against removing the final key: if the
change is an uncheck and prev contains only that slot, do not update state (keep
at least one selected). Update any related selection update paths that can
produce an empty array (selectedSlotKeys/timeSlots) so the table header colSpan
logic (where colSpan is derived) never receives 0.
- Around line 739-744: The button element that serves as a popover trigger in
the PrintMedicationAdministration component currently lacks an explicit ARIA
label, making it inaccessible to screen readers. Add an aria-label attribute to
the button that provides contextual information about the medication, date, and
slot being represented by the checkmark. This label should convey what action
the button triggers and what medication/administration data it relates to,
ensuring screen reader users understand the purpose and context of the
interaction.
- Around line 53-58: formatSlotHour and several direct date/format(...) usages
hardcode English AM/PM and formats; replace these with locale-aware formatting:
use Intl.DateTimeFormat (or toLocaleTimeString/toLocaleDateString) with the app
locale (e.g., i18n.language) to produce the hour display instead of hardcoded
"AM"/"PM" and update all format(...) usages to pass the locale or use
IntlDateTimeFormatter; any visible strings like "AM"/"PM" or other date labels
must be routed through i18next (t(...)) where appropriate; specifically update
the formatSlotHour function to create a Date from the hour and return
Intl.DateTimeFormat(i18n.language, { hour: '2-digit', hour12: true
}).format(date) and change the other calls that use format(...) to use
toLocaleString/toLocaleDateString or Intl.DateTimeFormat with i18n.language,
keeping function names like formatSlotHour and existing format(...) call sites
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f975b18-2cab-4206-bc49-4a67d7d1affd
📒 Files selected for processing (2)
public/locale/en.jsonsrc/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx
| const formatSlotHour = (hour: number) => { | ||
| const normalized = hour % 24; | ||
| const period = normalized < 12 ? "AM" : "PM"; | ||
| const display = normalized % 12 === 0 ? 12 : normalized % 12; | ||
| return `${String(display).padStart(2, "0")} ${period}`; | ||
| }; |
There was a problem hiding this comment.
Localize time/date output instead of hardcoded English formats.
formatSlotHour hardcodes "AM"/"PM", and several format(...) calls render without locale. This will keep the print UI in English-style formats even when the app language changes.
Suggested fix
+ import { enIN } from "date-fns/locale";
+ // map i18n.language -> date-fns locale in project utility if available
+ const dateLocale = enIN;
- const formatSlotHour = (hour: number) => {
- const normalized = hour % 24;
- const period = normalized < 12 ? "AM" : "PM";
- const display = normalized % 12 === 0 ? 12 : normalized % 12;
- return `${String(display).padStart(2, "0")} ${period}`;
- };
+ const formatSlotHour = (hour: number, locale: string) => {
+ const d = new Date();
+ d.setHours(hour % 24, 0, 0, 0);
+ return new Intl.DateTimeFormat(locale, {
+ hour: "2-digit",
+ minute: "2-digit",
+ hour12: true,
+ }).format(d);
+ };
- {format(new Date(encounter.period.start), "dd MMM yyyy, EEEE")}
+ {format(new Date(encounter.period.start), "dd MMM yyyy, EEEE", { locale: dateLocale })}As per coding guidelines, "All user-facing strings must use i18next for internationalization" and "Use proper date/time formatting for multiple language support."
Also applies to: 343-344, 393-396, 609-610, 771-776
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx`
around lines 53 - 58, formatSlotHour and several direct date/format(...) usages
hardcode English AM/PM and formats; replace these with locale-aware formatting:
use Intl.DateTimeFormat (or toLocaleTimeString/toLocaleDateString) with the app
locale (e.g., i18n.language) to produce the hour display instead of hardcoded
"AM"/"PM" and update all format(...) usages to pass the locale or use
IntlDateTimeFormatter; any visible strings like "AM"/"PM" or other date labels
must be routed through i18next (t(...)) where appropriate; specifically update
the formatSlotHour function to create a Date from the hour and return
Intl.DateTimeFormat(i18n.language, { hour: '2-digit', hour12: true
}).format(date) and change the other calls that use format(...) to use
toLocaleString/toLocaleDateString or Intl.DateTimeFormat with i18n.language,
keeping function names like formatSlotHour and existing format(...) call sites
intact.
Source: Coding guidelines
| onCheckedChange={(value) => | ||
| setSelectedSlotKeys((prev) => | ||
| value | ||
| ? [...prev, slot.key] | ||
| : prev.filter((key) => key !== slot.key), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Prevent deselecting the last slot to avoid invalid table structure.
At Line 327, users can uncheck every slot, which makes timeSlots.length === 0; then Line 603 sets colSpan={0}, causing unstable/invalid header rendering and a broken chart layout.
Suggested fix
- onCheckedChange={(value) =>
- setSelectedSlotKeys((prev) =>
- value
- ? [...prev, slot.key]
- : prev.filter((key) => key !== slot.key),
- )
- }
+ onCheckedChange={(value) =>
+ setSelectedSlotKeys((prev) => {
+ if (value) {
+ return prev.includes(slot.key) ? prev : [...prev, slot.key];
+ }
+ if (prev.length === 1) return prev; // keep at least one slot selected
+ return prev.filter((key) => key !== slot.key);
+ })
+ }Also applies to: 603-607
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx`
around lines 327 - 333, The checkbox onCheckedChange currently allows removing
the last selected slot which leads to timeSlots.length === 0 and colSpan being
set to 0; change the handler that updates selectedSlotKeys (the onCheckedChange
using setSelectedSlotKeys) to guard against removing the final key: if the
change is an uncheck and prev contains only that slot, do not update state (keep
at least one selected). Update any related selection update paths that can
produce an empty array (selectedSlotKeys/timeSlots) so the table header colSpan
logic (where colSpan is derived) never receives 0.
| <Trans | ||
| i18nKey="bed_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="morning_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="afternoon_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="night_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Keep the administration-times legend in sync with selected slot filters.
This block always renders all 4 slot labels, even when the user filters to a subset. The table columns and legend can diverge, which is misleading in a medication print view.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx`
around lines 449 - 475, The legend currently always renders all four Trans
labels inside PrintMedicationAdministration; change it to render only the labels
that match the component's current slot filter state/prop (e.g.,
selectedSlots/slotFilters) so the legend stays in sync with the table columns.
Locate the legend block in PrintMedicationAdministration and wrap each Trans
(i18nKey="bed_time"/"morning_time"/"afternoon_time"/"night_time") in a
conditional that checks the slot is included in the active filter array
(preserve ordering and comma separators when multiple are shown). Ensure you use
the existing slot filter state/prop used by the table (the same variable that
determines which columns render) so both legend and columns stay consistent.
| <button | ||
| type="button" | ||
| className="flex h-full min-h-[28px] w-full cursor-pointer items-center justify-center transition-colors hover:bg-green-100 print:hidden" | ||
| > | ||
| {cellContent} | ||
| </button> |
There was a problem hiding this comment.
Add an explicit ARIA label for the checkmark-only popover trigger.
The button currently exposes only visual symbols (✓), so screen readers lack medication/date/slot context for the interaction.
Suggested fix
<button
type="button"
+ aria-label={t("med_admin_details_aria_label", {
+ medication: group.productName,
+ date: format(date, "dd MMM yyyy"),
+ slot: slot.abbreviation,
+ count: admins.length,
+ })}
className="flex h-full min-h-[28px] w-full cursor-pointer items-center justify-center transition-colors hover:bg-green-100 print:hidden"
>As per coding guidelines, "Add ARIA labels for critical medical data (patient names, vital signs, medication alerts)" and "Ensure keyboard navigation works in components."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| type="button" | |
| className="flex h-full min-h-[28px] w-full cursor-pointer items-center justify-center transition-colors hover:bg-green-100 print:hidden" | |
| > | |
| {cellContent} | |
| </button> | |
| <button | |
| type="button" | |
| aria-label={t("med_admin_details_aria_label", { | |
| medication: group.productName, | |
| date: format(date, "dd MMM yyyy"), | |
| slot: slot.abbreviation, | |
| count: admins.length, | |
| })} | |
| className="flex h-full min-h-[28px] w-full cursor-pointer items-center justify-center transition-colors hover:bg-green-100 print:hidden" | |
| > | |
| {cellContent} | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx`
around lines 739 - 744, The button element that serves as a popover trigger in
the PrintMedicationAdministration component currently lacks an explicit ARIA
label, making it inaccessible to screen readers. Add an aria-label attribute to
the button that provides contextual information about the medication, date, and
slot being represented by the checkmark. This label should convey what action
the button triggers and what medication/administration data it relates to,
ensuring screen reader users understand the purpose and context of the
interaction.
Source: Coding guidelines
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #9556 |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Deploying care-preview with
|
| Latest commit: |
0892337
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d91ea8d4.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://eng-521-redesign-drug-chart.care-preview-a7w.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR redesigns the medication administration print preview to match the new ENG-521 layout, including fixed administration time slots and a more information-dense header/footer suitable for printing.
Changes:
- Replaced variable “slots per day” logic with four fixed administration time slots (B/M/A/N) and a slot filter in the print preview UI.
- Updated the print header to include facility details, patient location breadcrumb, and a derived hospital identifier.
- Enhanced the drug chart rows to include dosage form, duration, and aggregated instructions/clinical notes per medication group.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/components/Medicine/MedicationAdministration/PrintMedicationAdministration.tsx | Reworks print layout, adds fixed administration slots + filtering, header breadcrumb/hospital ID, and richer row/footer content. |
| public/locale/en.json | Adds new i18n keys for administration time slots, filter label, hospital ID, and IP location label. |
| // Filter state | ||
| const [showDiscontinued, setShowDiscontinued] = useState(false); | ||
| const [slotsPerDay, setSlotsPerDay] = useState(4); | ||
| const [showDiscontinued] = useState(false); |
| {/* Administration time slot filter (screen only) */} | ||
| <div className="print:hidden rounded-lg bg-gray-50 p-4 mb-4"> | ||
| <p className="mb-3 text-sm font-medium text-gray-700"> | ||
| {t("administration_times")}: | ||
| </p> | ||
| <div className="flex flex-wrap gap-x-8 gap-y-3"> |
| <label | ||
| key={slot.key} | ||
| className="flex cursor-pointer items-start gap-2" | ||
| > | ||
| {num} | ||
| </Button> | ||
| ))} | ||
| </div> | ||
| <Checkbox | ||
| className="mt-0.5" | ||
| checked={checked} | ||
| onCheckedChange={(value) => | ||
| setSelectedSlotKeys((prev) => | ||
| value | ||
| ? [...prev, slot.key] | ||
| : prev.filter((key) => key !== slot.key), | ||
| ) | ||
| } | ||
| /> |
| <span className="font-semibold"> | ||
| {":"} | ||
| {encounter.patient.name} | ||
| </span> |
| <span className="font-semibold"> | ||
| {":"} | ||
| {format( | ||
| new Date(encounter.period.start), | ||
| "dd MMM yyyy, EEEE", | ||
| )} | ||
| </span> | ||
| </div> |
| <span className="font-semibold"> | ||
| {":"} | ||
| {`${formatPatientAge(encounter.patient, true)}, ${t( | ||
| `GENDER__${encounter.patient.gender}`, | ||
| )}`} | ||
| </span> |
| <span className="font-semibold"> | ||
| {":"} | ||
| {hospitalId} | ||
| </span> |
| <div className="mb-2 border border-gray-400 p-2 flex items-center justify-between"> | ||
| <span className="text-gray-950 text-xl font-semibold "> | ||
| {t("regular_medications")} | ||
| </span> | ||
| <div className="text-gray-600 text-base"> | ||
| <Trans | ||
| i18nKey="bed_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="morning_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="afternoon_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| ,{" "} | ||
| <Trans | ||
| i18nKey="night_time" | ||
| components={{ | ||
| strong: <span className="text-gray-950" />, | ||
| }} | ||
| /> | ||
| </div> |
| // Filter state | ||
| const [showDiscontinued, setShowDiscontinued] = useState(false); | ||
| const [slotsPerDay, setSlotsPerDay] = useState(4); | ||
| const [showDiscontinued] = useState(false); |
There was a problem hiding this comment.
hmm, this breaks the discontinued filter/user can now see discontinued medications by default
|
|
||
| // Collect patient-facing instructions and clinical notes across every request | ||
| // in a medication group, de-duplicated for a compact footer strip. | ||
| const collectInstructionsAndNotes = (group: GroupedMedication) => { |
There was a problem hiding this comment.
do note that we can have note for each medication, but also a note for the prescription itself. In the latter case, doctor might add specific instructions on how prescribed medications might interact with each other (i.e. discontinue one if the other causes side effects for example).
So we will need to show both.
| const form = | ||
| latestRequest.requested_product?.definitional?.dosage_form | ||
| ?.display; | ||
| const { instructions: patientInstructions, notes } = |
There was a problem hiding this comment.
make it clearer - might want to rename this since it combines patient instructions and additional instructions (we don't currently use the former in UI).
| colSpan={totalColumns - 1} | ||
| className="border-b border-gray-300 bg-gray-50/70 px-2 py-1 text-xs leading-snug text-gray-600" | ||
| > | ||
| {patientInstructions.length > 0 && ( |
There was a problem hiding this comment.
When we have multi-dosage, joining instructions isn't ideal.
Jacobjeevan
left a comment
Jacobjeevan
left a comment
There was a problem hiding this comment.
There's an additional issue with this design as it doesn't allow user to print charts for the entire day. Here's what it looks like in previous design:
Proposed Changes
Fix ENG-521
Screenshot:
figma link: https://www.figma.com/design/Z93EYKSa1MdBmXndsMBJSQ/Care---Master?node-id=7346-4302&t=N7lK1t394FeEWD6j-1
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit