(fix) O3-5831: Fix patient banner building blocks at narrow widths - #1820
Conversation
|
Greptile SummaryThis PR improves narrow patient-banner layouts and adds a primary-only identifier display mode.
|
| Filename | Overview |
|---|---|
| packages/framework/esm-styleguide/src/patient-banner/patient-info/patient-banner-patient-identifiers.component.tsx | Adds primary-only filtering and grouped separators, but collapsed mode drops all identifiers while primary metadata is unavailable. |
| packages/framework/esm-styleguide/src/patient-banner/patient-info/patient-banner-patient-info.component.tsx | Exposes the identifier visibility option and groups separators with their following demographic items. |
| packages/framework/esm-styleguide/src/patient-banner/patient-info/patient-banner-patient-info.module.scss | Adds wrapping and pseudo-element separator styles for narrow layouts. |
| packages/framework/esm-styleguide/src/patient-banner/contact-details/patient-banner-contact-details.module.scss | Adds container-query column stacking and relationship wrapping. |
| packages/framework/esm-styleguide/src/patient-photo/patient-photo.component.tsx | Limits generated initials to two characters using the first and last name parts. |
| packages/framework/esm-styleguide/src/patient-photo/patient-photo.test.tsx | Verifies first-and-last initials for multipart names. |
Reviews (1): Last reviewed commit: "(fix) O3-5831: Fix patient banner buildi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return code && !excludePatientIdentifierCodeTypes?.uuids.includes(code); | ||
| }) ?? []; | ||
| }) ?? [] | ||
| ).filter((identifier) => (showAllIdentifiers ? true : identifier.type?.coding?.[0]?.code === primaryIdentifierCode)); |
There was a problem hiding this comment.
When showAllIdentifiers is false and the primary identifier code is still loading or has no configured mapping, this filter rejects every identifier, causing the banner to display no patient identifier temporarily or permanently.
Artifacts
Repro: focused component test covering loading and unavailable primary-code metadata
- Evidence file captured while the check ran.
Repro: narrow Vitest configuration used to execute the component test
- Evidence file captured while the check ran.
Repro: verbose Vitest output showing supplied identifiers and empty rendered DOM in both cases
- The full command output behind this check.
There was a problem hiding this comment.
Still present at the head commit, and the blast radius is a bit wider than the loading window. I ran the component with showAllIdentifiers={false} against five states, and every one renders an empty DOM with no identifier at all: primaryIdentifierCode undefined while SWR is in flight, undefined because emr.primaryIdentifierType has no metadata mapping, undefined after the lookup errors, a patient whose identifiers simply don't include the primary type, and a primary type that excludePatientIdentifierCodeTypes already filtered out.
The true default is the only reason this isn't live today, which also means no unit test or E2E run can reach it, and there's no test covering either direction of the prop.
The code as it stands degrades gracefully when primaryIdentifierCode is missing: everything falls through to SecondaryIdentifier, so nothing vanishes. I'd keep that guarantee by falling back to the first available identifier instead of to none. Dropping the trailing .filter(...) and deriving the visible set separately does it (the JSX below then maps visibleIdentifiers):
const primaryIdentifiers = filteredIdentifiers.filter(
(identifier) => identifier.type?.coding?.[0]?.code === primaryIdentifierCode,
);
const visibleIdentifiers = showAllIdentifiers
? filteredIdentifiers
: primaryIdentifiers.length > 0
? primaryIdentifiers
: filteredIdentifiers.slice(0, 1);I patched that in locally and all five cases render an identifier again, with the existing 15 tests in the banner and photo suites still green. Gating on isLoading on its own won't cover it, since the missing-mapping and no-primary-identifier cases leave the code undefined with isLoading false.
This does need sorting before merge: on a server without that metadata mapping, a collapsed banner shows a patient with no identifier at all, nothing throws, and the first person to notice is whoever is reading the chart.
There was a problem hiding this comment.
The analysis is spot-on, and the proposed fix is correct. The single-line .filter(...) chain leaves no fallback when primaryIdentifierCode is undefined or unmatched — all five failure modes you listed produce an empty array.
The visibleIdentifiers approach handles all of them cleanly:
| ).filter((identifier) => (showAllIdentifiers ? true : identifier.type?.coding?.[0]?.code === primaryIdentifierCode)); | |
| const filteredIdentifiers = ( | |
| identifiers?.filter((identifier) => { | |
| const code = identifier.type?.coding?.[0]?.code; | |
| return code && !excludePatientIdentifierCodeTypes?.uuids.includes(code); | |
| }) ?? [] | |
| ); | |
| const primaryIdentifiers = filteredIdentifiers.filter( | |
| (identifier) => identifier.type?.coding?.[0]?.code === primaryIdentifierCode, | |
| ); | |
| const visibleIdentifiers = showAllIdentifiers | |
| ? filteredIdentifiers | |
| : primaryIdentifiers.length > 0 | |
| ? primaryIdentifiers | |
| : filteredIdentifiers.slice(0, 1); |
Then replace filteredIdentifiers with visibleIdentifiers in the JSX map. The .slice(0, 1) fallback ensures a collapsed banner always shows at least something — matching the current showAllIdentifiers={true} guarantee — regardless of whether primaryIdentifierCode is still loading, unconfigured, or excluded.
One small addition worth having: a test that passes showAllIdentifiers={false} with primaryIdentifierCode undefined (or returning no match) and asserts that the first identifier still renders. That would have caught this before review and covers the regression path going forward.
4ad45f6 to
a5edd49
Compare
|
@denniskigen could you please review this PR in regards to the patient banner building blocks break down at narrow widths |
|
@dkayiwa could you please review this. |
| function getInitials(name: string, maxInitials = 2): string { | ||
| const parts = name.split(/\s+/).filter(Boolean); | ||
| const selected = parts.length > maxInitials ? [parts[0], parts.at(-1)] : parts; | ||
| return selected.map((part) => part![0]).join(''); | ||
| } |
There was a problem hiding this comment.
maxInitials no longer means "at most this many initials": getInitials(name, 4) on a five-part name returns two, not four. It reads as a threshold now, and the only call site (line 104) passes no second argument, so the parameter is mostly something for the next reader to puzzle over. Dropping it also removes the need for the non-null assertion.
| function getInitials(name: string, maxInitials = 2): string { | |
| const parts = name.split(/\s+/).filter(Boolean); | |
| const selected = parts.length > maxInitials ? [parts[0], parts.at(-1)] : parts; | |
| return selected.map((part) => part![0]).join(''); | |
| } | |
| function getInitials(name: string): string { | |
| const parts = name.split(/\s+/).filter(Boolean); | |
| const selected = parts.length > 2 ? [parts[0], parts[parts.length - 1]] : parts; | |
| return selected.map((part) => part[0]).join(''); | |
| } |
Identical output for every input, so take it or leave it.
| .withSeparator { | ||
| &::before { | ||
| content: '\00B7'; | ||
| margin: 0 layout.$spacing-02; | ||
| } | ||
| } |
There was a problem hiding this comment.
.separator further down (line 137) has no callers left, now that both of its usages became this pseudo-element, so the file is left with two separator styles and one of them dead. Worth deleting it in the same pass. Not something I'd hold the PR for on its own.
67feefc to
46f2d9a
Compare
solomonfortune
left a comment
solomonfortune
left a comment
There was a problem hiding this comment.
@dkayiwa i have applied the fix exactly as suggested i.e. visibleIdentifiers now falls back to the first identifier when primaryIdentifierCode is unresolved, rather than filtering to empty. Also added the regression test you flagged: showAllIdentifiers={false} with primaryIdentifierCode undefined now asserts the first identifier still renders. All 11 tests in the patient-banner suite pass.
There was a problem hiding this comment.
Thanks for working on this, @solomonfortune. I've left a few suggestions below.
| .withSeparator { | ||
| &::before { | ||
| content: '\00B7'; | ||
| margin: 0 layout.$spacing-02; | ||
| } | ||
| } |
There was a problem hiding this comment.
| .withSeparator { | |
| &::before { | |
| content: '\00B7'; | |
| margin: 0 layout.$spacing-02; | |
| } | |
| } | |
| .withSeparator:not(:first-child)::before { | |
| content: '\00B7'; | |
| margin-inline-end: layout.$spacing-02; | |
| } |
Letting the selector decide when the dot shows means we don't need showLeadingSeparator at all. It also fixes the spacing. margin: 0 $spacing-02 stacks on top of the .demographics gap, so the dot currently gets 8px on its left and 4px on its right. Main is 6px both sides.
| interface PatientBannerPatientIdentifiersProps { | ||
| identifiers: fhir.Identifier[] | undefined; | ||
| showIdentifierLabel: boolean; | ||
| showLeadingSeparator?: boolean; | ||
| showAllIdentifiers?: boolean; | ||
| } |
There was a problem hiding this comment.
| interface PatientBannerPatientIdentifiersProps { | |
| identifiers: fhir.Identifier[] | undefined; | |
| showIdentifierLabel: boolean; | |
| showLeadingSeparator?: boolean; | |
| showAllIdentifiers?: boolean; | |
| } | |
| interface PatientBannerPatientIdentifiersProps { | |
| identifiers: fhir.Identifier[] | undefined; | |
| showIdentifierLabel: boolean; | |
| showAllIdentifiers?: boolean; | |
| } |
Both components go out through @openmrs/esm-framework, so this becomes public API that only its sibling component would ever set. The destructured default below and the conditional class can go with it:
<span key={value} className={classNames(styles.identifier, styles.withSeparator)}>| <PatientBannerPatientIdentifiers | ||
| identifiers={patient.identifier} | ||
| showIdentifierLabel | ||
| showLeadingSeparator={Boolean(patient.birthDate)} |
There was a problem hiding this comment.
| showLeadingSeparator={Boolean(patient.birthDate)} |
Same as above, showLeadingSeparator={Boolean(patient.birthDate)} can go once the separator is selector-driven.
| it('shows only the primary identifier when showAllIdentifiers is false and the primary identifier code resolves', () => { | ||
| render( | ||
| <PatientBannerPatientIdentifiers identifiers={mockIdentifiers} showIdentifierLabel showAllIdentifiers={false} />, | ||
| ); | ||
|
|
||
| expect(screen.getByText(/openmrs id/i)).toBeInTheDocument(); | ||
| expect(screen.getByText(/100gej/i)).toBeInTheDocument(); | ||
| expect(screen.queryByText(/national id/i)).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
| it('shows only the primary identifier when showAllIdentifiers is false and the primary identifier code resolves', () => { | |
| render( | |
| <PatientBannerPatientIdentifiers identifiers={mockIdentifiers} showIdentifierLabel showAllIdentifiers={false} />, | |
| ); | |
| expect(screen.getByText(/openmrs id/i)).toBeInTheDocument(); | |
| expect(screen.getByText(/100gej/i)).toBeInTheDocument(); | |
| expect(screen.queryByText(/national id/i)).not.toBeInTheDocument(); | |
| }); | |
| it('shows only the primary identifier when showAllIdentifiers is false and the primary identifier code resolves', () => { | |
| mockUsePrimaryIdentifierCode.mockReturnValue({ | |
| primaryIdentifierCode: '4281ec43-388b-4c25-8bb2-deaff0867b2c', | |
| isLoading: false, | |
| error: undefined, | |
| }); | |
| render( | |
| <PatientBannerPatientIdentifiers identifiers={mockIdentifiers} showIdentifierLabel showAllIdentifiers={false} />, | |
| ); | |
| expect(screen.getByText(/national id/i)).toBeInTheDocument(); | |
| expect(screen.getByText(/123456789/i)).toBeInTheDocument(); | |
| expect(screen.queryByText(/openmrs id/i)).not.toBeInTheDocument(); | |
| }); |
This one can't fail as written. The primary identifier is also mockIdentifiers[0], so it passes whether we pick the primary or just take the first one. I swapped the branch for a bare filteredIdentifiers.slice(0, 1) and all six tests still passed. Making the primary the second identifier makes it fail on that mutation.
46f2d9a to
729747a
Compare
|
solomonfortune
left a comment
solomonfortune
left a comment
There was a problem hiding this comment.
@denniskigen Separator dots are now selector-driven (:not(:first-child)::before); showLeadingSeparator removed from the interface and call site. Fixed the dot spacing. Rewrote the flagged test so it fails if primary vs. first gets swapped and all 11 tests pass locally.
|
@dkayiwa please review this PR at your time of convenience. |
3 participants
Requirements
Summary
Fixes five patient banner bugs that surface at narrow widths:
showAllIdentifiersprop (defaulttrue, non-breaking) to show only the primary identifier when collapsed. Wiring to patient-chart's actual collapse/expand state is a follow-up ticket.Open question: designs show primary identifier as plain text (collapsed) vs. tag (expanded); this PR keeps it a tag in both states pending design input, to avoid flicker on toggle.
CI note:
@openmrs/esm-utils#testfails intermittently under the local Turbo pre-push hook but passes 161/161 in isolation — appears to be pre-existing flakiness unrelated to this diff.Screenshots
1. Contact details stacking
2. Long name wrapping
3. Avatar initials capped
4. Separator dot wrapping
5. showAllIdentifiers
Related Issue
https://issues.openmrs.org/browse/O3-5831
Other
N/A