feat(xo-core): add pool status item component #7769
Conversation
P4l0m4
commented
Jun 21, 2024
P4l0m4
commented
| import type { IconDefinition } from '@fortawesome/fontawesome-common-types' | ||
| import { faAngleRight } from '@fortawesome/free-solid-svg-icons' | ||
|
|
||
| type LegendColor = 'default' | 'success' | 'warning' | 'error' | 'disabled' | 'dark-blue' |
There was a problem hiding this comment.
Why not extract the type from the UiLegend component into a ui-legend.type.ts and import the type from that file?
| type Segment = { | ||
| value: number | ||
| color: 'success' | 'warning' | 'error' | 'unknown' | ||
| } |
There was a problem hiding this comment.
Same here, you can create donut-chart.type.ts and import the type instead of duplicating it.
|
|
||
| interface Props { | ||
| title: string | ||
| titleLink: string |
There was a problem hiding this comment.
According to the design system, the link should be optional.
| titleLinkLabel: 'See all', | ||
| totalLabel: 'Total', |
There was a problem hiding this comment.
These strings should be translated. Also, it doesn't make sense to have the link required, and the link label optional with a default value.
| <div class="total"> | ||
| <span class="typo c2-semi-bold">{{ totalLabel }}</span | ||
| ><span class="typo h3-semi-bold">{{ totalValue }}</span> | ||
| </div> |
There was a problem hiding this comment.
Why not use the CardNumbers component? Also, according to the mock-ups, the whole component should be right-aligned on desktop and left-aligned on mobile.
There was a problem hiding this comment.
Someone said the design system prevails on the mockup. On the design system, the label and value are spread appart
There was a problem hiding this comment.
As I told you before here (point #4) or there, this has been discussed during the call with Clémence in which you took part.
You already made this exact same change twice.
Reusable components are useless if they are not intended to be reused.
There was a problem hiding this comment.
In my opinion, this component is not useful in web-core. According to the mock-ups, the component will only be used in XO 6. I would suggest to move the component to web and add the business logic to get the data from the pool store. There is no need to make this component act as an intermediate component that redefines all the props needed by the child components. This would create unnecessary maintainability problems. By the way, since the values needed by DonutChart and UiLegend are mostly the same, maybe you could make a child component for this part.
| <RouterLink :to="titleLink"> | ||
| <UiButton level="tertiary" size="extra-small" :right-icon="faAngleRight">{{ titleLinkLabel }}</UiButton> | ||
| </RouterLink> |
There was a problem hiding this comment.
RouterLink renders a <a> tag, and <button> is not allowed as a child.
P4l0m4
force-pushed
the
pool-status-item-component
branch
from
32a7744
to
ca5a757
Compare
P4l0m4
force-pushed
the
pool-status-item-component
branch
from
ca5a757
to
a56bbe8
Compare
|
Note I'll wait for #7778 before doing this review. |
