-
Notifications
You must be signed in to change notification settings - Fork 248
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
(fix) Allow cancelling orders and address various small fixes #1899
Conversation
Size Change: -202 kB (-1.78%) Total Size: 11.1 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.
Mostly LGTM. I've left some minor suggestions.
packages/esm-patient-orders-app/src/components/order-details-table.scss
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/components/order-details-table.scss
Outdated
Show resolved
Hide resolved
packages/esm-patient-orders-app/src/components/orders-details-table.component.tsx
Show resolved
Hide resolved
@@ -328,7 +329,7 @@ const OrderDetailsTable: React.FC<OrderDetailsProps> = ({ title, patientUuid, sh | |||
{showAddButton && ( | |||
<Button | |||
kind="ghost" | |||
renderIcon={(props) => <Add size={16} {...props} />} | |||
renderIcon={() => <AddIcon size={16} />} |
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.
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.
Aha! The props are important. I'll look into the color thing, but I think that's best fixed in the framework
packages/esm-patient-orders-app/src/components/orders-details-table.component.tsx
Outdated
Show resolved
Hide resolved
8f5a22b
to
7d31ac0
Compare
} | ||
} | ||
|
||
.statusPill { |
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.
💯
@@ -317,7 +318,7 @@ const OrderDetailsTable: React.FC<OrderDetailsProps> = ({ title, patientUuid, sh | |||
{showPrintButton && ( | |||
<Button | |||
kind="ghost" | |||
renderIcon={(props) => <Printer size={16} {...props} />} | |||
renderIcon={(props) => <PrinterIcon {...props} size={16} />} |
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.
Out of curiosity, is the design for iconography from the framework meant to inherit designs from the parent or do they need to be passed? for example in this case the Button stylings like color?
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.
They should inherit them, but I'm still untangling the mess of CSS to get things right. In this case, Carbon defines a selector like this:
.cds--btn--ghost:not([disabled]) svg {
fill: var(--cds-icon-primary, #161616);
}
But also setting fill="currentColor"
on the icon, so it effectively ignores the CSS. Originally, I was doing something similar too, but it caused issues with getting the color right in some other cases. I'm still fiddling with getting this working in all cases...
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.
The props
here are basically passing in some additional padding and the iconDescription
as an Aria label.
Requirements
Summary
This PR does various minor fixes to the order details table:
Screenshots
Related Issue
Other