(feat) Add icons for various order types #1259
Conversation
vasharma05
force-pushed
the
feat/add-order-pictograms
branch
from
26151fa to
3b0e489
Compare
vasharma05
requested review from
denniskigen,
ibacher and
usamaidrsk
and removed request for
denniskigen
|
Size Change: -145 kB (-2.28%) Total Size: 6.2 MB
ℹ️ View Unchanged
|
|
So, this needs a few changes.
|
|
Hi @ibacher!
Yeah, I tried removing the size factor from these SVGs, but wasn't being able to do the same.
Yes, they should be uncolored, but as per designs for the order basket, these pictograms are colored, I have asked for these icons from this slack thread: https://openmrs.slack.com/archives/C06SSBQ7FPU/p1733426639388409 Thanks! |
vasharma05
changed the title
Basically, the problem here is that neither the pictogram nor icon setups are really that adaptable. The SVGs need to be in the expected viewport size or else they won't render correctly. So either we need versions of these upscaled to a 80x80 view box (sorry, that's the actual view port size of the pictograms) or downscaled to a 16x16 view box or we need a new custom component for images with a viewbox sized to the size of these. These won't render correctly without one of those three things being done.
Hmmm... maybe we should treat them more similarly to pictograms then. They'll still need to be either resized or will need a custom component, I think to make things work correctly. |
|
Hi @ibacher, as I confirmed from the designs, these icons are meant to be 24px in size, instead of 16px. |
|
@vasharma05 The size that they are meant to displayed at here is irrelevant. We can support that. It's an SVG. The problem is that the What's going to happen is that the Similarly, the Basically, our SVGs need to be in standardized view boxes for the icon and pictogram framework to work. This is also true of, e.g., Carbon's icons, which are rendered in a 20x20 view box, but can be scaled to whatever number of pixels is required. |
|
Hi @ibacher, I have updated the icons and have also added icon for Drug order |
There was a problem hiding this comment.
Isn't this just a colorized version of user-xray? I'm not sure we need this.
There was a problem hiding this comment.
This actually makes me think we should go back to stripping the fills from these icons and set them at the point we're rendering them and need the colors (none of these icons have more than two colors, so we can handle it as fill and background-color.
There was a problem hiding this comment.
Hi @ibacher, I accept that we can fill the colors using the background-color and fill. But, I have 2 points to consider, 1. These icons are specific to the order basket, and secondly, we are able to add the order types in the order basket via configuration, hence defining the background-color and fill will be something that will have to handled by configuration. Something similar being faced with the order type colors, for which I created a PR here: openmrs/openmrs-esm-patient-chart#2188
Either we don't put any colors in the SVGs, or we should keep the icons as is. I am hoping to pass SVG registration name in the configuration as is implemented here and here. Similar are implemented for General Order types.
Thanks!
There was a problem hiding this comment.
Also to add to the above points, the icons will have to modified, because the icon size is smaller than the viewbox to fill in the background colors, hence these icons will appear smaller than the rest of the icons, something which will have to be taken care by the developer implementing these icons, instead of using them out of styleguide directly.
Thanks!
There was a problem hiding this comment.
Ian, your final thoughts on this?
There was a problem hiding this comment.
I honestly don't think this "new order types through configuration" is a fully-baked idea. Even though it's achievable in the order basket, I can't think of a scenario where we don't also need a paired app to handle tracking the orders or at least ensuring they end up somewhere, like we do with medications and tests. And once we do that, the need for defining order types via configuration just seems to be creating work in two different places. This whole approach to me looks like a first draft that's going to need to be heavily revised. (The only example I can think of is a maybe a referral order which transfers at least an aspect of a patient's care to another clinic or organization).
Each order type is handled via a different workflow, usually that goes to different staff to handle who do not need to interact with the rest of the system. E.g., a radiology order goes to the imaging department which only handles imaging orders. They'd need something like the dispensing app: a specific view that queues up imaging orders that have been placed, tracks certain patient data and allows them to submit the report of the reading, and potentially a link to the PACS. Procedure orders, meanwhile, are handled by the provider performing the procedure. For surgical orders, the first thing they may need to do is locate an appropriate OR on the schedule, add a slot to the schedule, perform the procedure and then report any outcomes (which may be notes or results depending on the procedure type.
In short, I don't see how we build something that's long-term usable without building a focused app for it and, at that point, I don't see the value proposition of being able to define orders via configuration.
There was a problem hiding this comment.
the icon size is smaller than the viewbox to fill in the background colors
Every icon has a certain amount of padding. All we've done is remove colors, not change the drawing. Note that every icon we get is always colored, they're just usually black.
These icons are specific to the order basket
In what way? These icons are being used in the order basket in the designs, but the actual images are not necessarily unique to the order basket, thus, for example, thisimaging-order icon is the exact image that we have as the user-xray icon, except that one is colored green and the other isn't and I think that applies to all of these—even if we don't use them yet, there's no reason that, e.g., the Rx icon is only going to be used here.
I also want to point out that if the argument here is that we need to support users adding custom order types, then we want to make it as flexible as possible for users to add icons for those new order types, i.e., it makes it ridiculous if I can add an order type via config, but get the icon for it, I need to make a PR to update the styleguide. So that seems to suggest that we really should support specifying the background and fill colors via configuration, because then I can define an order type and reuse an existing icon for it.
I'm not trying to be difficult here; I'm trying to make sure that we're not committing stuff that's going to require lots of churn. O3 is meant to be becoming stable.
There was a problem hiding this comment.
I honestly don't think this "new order types through configuration" is a fully-baked idea. Even though it's achievable in the order basket, I can't think of a scenario where we don't also need a paired app to handle tracking the orders or at least ensuring they end up somewhere, like we do with medications and tests. And once we do that, the need for defining order types via configuration just seems to be creating work in two different places. This whole approach to me looks like a first draft that's going to need to be heavily revised. (The only example I can think of is a maybe a referral order which transfers at least an aspect of a patient's care to another clinic or organization).
Each order type is handled via a different workflow, usually that goes to different staff to handle who do not need to interact with the rest of the system. E.g., a radiology order goes to the imaging department which only handles imaging orders. They'd need something like the dispensing app: a specific view that queues up imaging orders that have been placed, tracks certain patient data and allows them to submit the report of the reading, and potentially a link to the PACS. Procedure orders, meanwhile, are handled by the provider performing the procedure. For surgical orders, the first thing they may need to do is locate an appropriate OR on the schedule, add a slot to the schedule, perform the procedure and then report any outcomes (which may be notes or results depending on the procedure type.
In short, I don't see how we build something that's long-term usable without building a focused app for it and, at that point, I don't see the value proposition of being able to define orders via configuration.
There was a problem hiding this comment.
the icon size is smaller than the viewbox to fill in the background colors
Every icon has a certain amount of padding. All we've done is remove colors, not change the drawing. Note that every icon we get is always colored, they're just usually black.
These icons are specific to the order basket
In what way? These icons are being used in the order basket in the designs, but the actual images are not necessarily unique to the order basket, thus, for example, thisimaging-order icon is the exact image that we have as the user-xray icon, except that one is colored green and the other isn't and I think that applies to all of these—even if we don't use them yet, there's no reason that, e.g., the Rx icon is only going to be used here.
I also want to point out that if the argument here is that we need to support users adding custom order types, then we want to make it as flexible as possible for users to add icons for those new order types, i.e., it makes it ridiculous if I can add an order type via config, but get the icon for it, I need to make a PR to update the styleguide. So that seems to suggest that we really should support specifying the background and fill colors via configuration, because then I can define an order type and reuse an existing icon for it.
I'm not trying to be difficult here; I'm trying to make sure that we're not committing stuff that's going to require lots of churn. O3 is meant to be becoming stable.
| export const ImagingOrderIcon = () => <span>ImagingOrderIcon</span>; | ||
| export const MaterialOrderIcon = () => <span>MaterialOrderIcon</span>; | ||
| export const ProcedureOrderIcon = () => <span>ProcedureOrderIcon</span>; | ||
| export const ReferralOrderIcon = () => <span>ReferralOrderIcon</span>; | ||
| export const DrugOrderIcon = () => <span>DrugOrderIcon</span>; |
There was a problem hiding this comment.
Can we keep these alphabetized? It makes it easier for us to ensure that all the icons are created.
| */ | ||
| export const DrugOrderIcon = memo( | ||
| forwardRef<SVGSVGElement, IconProps>(function DrugOrderIcon(props, ref) { | ||
| return <Icon ref={ref} icon="omrs-icon-drug-order" iconProps={props} />; |
There was a problem hiding this comment.
If we're adding colorized icons, I can see having a new naming convention for the colored versions. However, we should be consistent...
|
To be clear about the status here, I'll approve this if you can address the two small technical comments. |
|
Yes @ibacher , I'll make the changes. Thanks a lot for the above explanations, I align with them. |
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This PR adds icons for drug, imaging, procedure, material and referral order types.
Screenshots
None
Related Issue
None
Other