-
Notifications
You must be signed in to change notification settings - Fork 234
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
BREAKING: Rework card config system (supports O3-3242) #1263
Conversation
Size Change: -7.12 kB (-0.12%) Total Size: 5.89 MB ℹ️ View Unchanged
|
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 simplifies the config a bit. Some things here don't feel right, but I think they are fixable.
Previously, I envisioned the config system to allow for arbitrary number of rows, each row can have any of the predefined elements, or even extension elements (not yet implemented). A custom "row" can technically be implemented as a row with an extension element. If I understand correctly, the changes in this PR are:
- we treat each element type defined here as first-class objects. They are unique enough to define even their own props; the logic of what to passed in is taken care of by the unique cases in the big switch statement. (I'd prefer that all take in a common set of props, but I don't feel too strongly about it)
- The header and footer rows get first class support. All other rows sandwiched in between are to be added as extensions.
- The
WardPatientCodedObsTags
is so specific and unique that it's elevated from being an element to being an extension row, with its own extension config. (This feels strange, but again I don't feel too strongly about it)
Some questions I have:
- How would someone add an extension element to the header and footer? (somewhat likely) We need to consider what props we should pass into it, and how we define the extension slot name.
- How would someone add predefined elements into the sandwiched extension rows? (perhaps less likely) Would it only be possible if we also make the predefined elements available as extensions?
packages/esm-ward-app/src/config-schema-extension-colored-obs-tags.ts
Outdated
Show resolved
Hide resolved
@@ -207,23 +149,33 @@ export interface WardConfigObject { | |||
} | |||
|
|||
export interface WardPatientCardsConfig { | |||
patientCardElementDefinitions: Array<PatientCardElementDefinition>; | |||
obsElementDefinitions: Array<ObsElementDefinition>; | |||
identifierElementDefinitions: Array<IdentifierElementDefinition>; |
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 think patientAddressElementFields
should belong here instead of the WardPatientCardDefinition
interface.
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.
Sounds good. That does mean that the same address elements will necessarily be used for every ward, but that's probably fine.
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.
Wait... why? Each value in obsElementDefinitions
and identifierElementDefinitions
can specify which element id to apply the config to. Can't we just do the same thing with patientAddressElementFields
?
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.
Oh I see, is what you're suggesting that instead of having 'patient-address' be a built-in element that is configurable, that instead we should provide a system for defining elements of type 'patient-address' which can then be included, but where the only real difference between the different patient-address elements is which fields are shown?
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. That follows the same pattern as the identiferElement
and obsElement
, right?
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.
Sure, it does.
if (obsConfig) { | ||
return <WardPatientObs patient={patient} visit={visit} config={obsConfig} />; | ||
} else if (idConfig) { | ||
return <WardPatientIdentifier patient={patient} config={idConfig} />; |
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 feels off to have WardPatientIdentifier
appear twice in this switch statement, one for having it take the default config and one for having it take a custom config.
I think the id
field of obsElementDefinition
/ identifierElementDefinition
is supposed to specify which element it configures. Can we have the id
be nullable, in which case it applies the config to the default element (patient-identifier / patient-obs)? I think this should simplify the logic 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.
No, the config system doesn't support types being nullable. null
can be provided as a default value, but an element with type Type.String
cannot have null
provided as the value by configuration.
I'm not sure what feels off about this, but it seems like a more intuitive API to provide than one in which there is a magic value of id
that modifies the default identifier element.
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.
Oh I think I figured out a slightly better way about it
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 feel I'm well-placed to evaluate this commit. However, broadly speaking, I'm in favour of anything that keeps the configuration relatively simple as a design goal of O3 is to have it configurable by implementers with minimal programming knowledge.
return WardPatientBedNumber; | ||
case 'patient-name': | ||
return WardPatientName; | ||
export const WardPatientCardElement: React.FC<{ elementId: string; patient: Patient; visit: Visit }> = ({ |
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.
Minor, but something which renders a React element, as opposed to data-fetching, feels like it should be in a .component.tsx
file rather than a .resource.tsx
file (even though this MFE doesn't do a great job of following the Coding Conventions).
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 certainly, lots of files will get renamed after the dust settles a little, before I merge this PR.
I'll generally defer to @chibongho has he is more up-to-speed on the original design of this, but I'll keep an eye on the PR and weigh in on things I see. A few thoughts:
|
Thanks so much for your thoughtful reviews, @chibongho , @mogoodrich , and @ibacher .
Yeah, the idea is that each row is an extension—so the baby row, when it's time to add that, will also be an extension.
Good thought! I've added extension slots for those.
Right now the only rows that are in any designs I've seen are quite different than the contents of the header and footer. The expectation encoded in this approach is that rows will be coded from scratch. I think this is the most appropriate way about it, because all of the existing row designs are highly idiosyncratic, and each row is pretty small and self-contained, easy to build from scratch. If someone really needs a row that displays the value of an obs in plain text, and for some reason they don't want to just put it in the header, they'll have to write a fetcher for that obs, and that's probably okay. It won't pollute any other code anyway. |
Ok, I've updated the address and identifier configurations. I'll fix the merge conflicts shortly. |
b73b046
to
e538700
Compare
045c335
to
95ab9e0
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.
Looks ok. Pretty scary change TBH. Hoping that nothing breaks (or that they are easy fixes).
encounterAssigningToCurrentInpatientLocation={encounterAssigningToCurrentInpatientLocation} | ||
/> | ||
))} | ||
<ExtensionSlot name={headerExtensionSlotName} state={extensionSlotState} /> |
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 was hoping there is an ExtensionElement
that can be placed with other elements in arbitrary instead of at the end, but we can do that in a different PR.
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 understand, could you elaborate on this?
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.
Sorry, typo. should be "in arbitrary order". So we can have, for example, bed-number
, patient-name
, extension-element
, patient-identifier
(instead of having the extension-element
at the end)
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.
If that becomes a real need, the thing to do would probably be to turn the elements that come after the patient name into extensions and have an extension slot come after name. I would suggest not treating bed number and patient name as arbitrary "bento box" elements, but rather as things that are managed by the card itself. So in the main card you'd have
bed number | patient name | header extension slot
And then in the admission request card you'd have
patient name | header extension slot
And then all of those elements, obs display etc, would be configurable directly using extension config (so the card wouldn't have anything to do with their configuration), and you'd just use the config system to add/remove/rearrange them.
But that would come with some performance cost (which would be significantly lightened by O3-1418). So I'd recommend not doing that until it's clear that there is a use case.
I've updated the config in the PR description; that should work. Thanks @chibongho ! |
@brandones the new config seems to work. thanks! |
Requirements
Summary
Simplifies the config schema. No more polymorphic config objects. Makes each card row other than the header and footer into an extension. Right now there's just the "coloredObsTags" extension, which is used for displaying risk factors.
This means that card rows should be configured using the extension configuration system. This is the one thing that is a little less intuitive than it was before, since the row config lives somewhere separate from the card config. However, what we get from the trade-off is that new types of rows can be introduced by third-party modules without needing to modify esm-ward-app.
The new KGH config should be
No UI change.
Screenshots
Related Issue
https://issues.openmrs.org/browse/O3-3242
Other
I'm not 100% sure the admission-reason config is working & correct because none of the admitted patients seem to have the relevant obs.
It wouldn't be unreasonable, I think, to rename
colored-obs-tags-card-row
tocolored-obs-tags
, since there's actually nothing very specific to the card row situation about it. It could just as easily be dropped in to lots of places. The card itself is providing the row styling.NOTE: I've left all the file names intact in order to facilitate reviewing. Once this approach is something we're happy moving forward with, I'll rename files and folders into a structure more appropriate for the new setup.