-
Notifications
You must be signed in to change notification settings - Fork 75
Main action cards #1278
base: dev
Are you sure you want to change the base?
Main action cards #1278
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -0,0 +1,235 @@ | |||
import { Badge, Box, Button, Flex, Heading, Stack, Text } from '@chakra-ui/react'; | |||
import React, { PropsWithChildren, ReactNode } from 'react'; | |||
import { getVariant } from '../c-ratio-health-card/getVariant'; |
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 function is probably gonna be important in a few places. Maybe we should think about a better name and put it elsewhere.
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 for sure. Maybe something like solvency/health state?
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.
Just keep it in v2-components for now? Maybe getHealthVariant
since we have color and components variants defined with these values?
hmm a little more work on the disabled states coming |
border={'1px'} | ||
borderColor={variant} | ||
display="flex" | ||
alignItems={'center'} |
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's a bit annoying but can we do just "center" and "1px"
My editor is code completing to {'center'} as well but bit annoying tbh
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.
yea I dont care too much. But if we wanna enforce it I'll add 'react/jsx-curly-brace-presence': ['warn', { props: 'never', children: 'never' }],
. Then eslint can fix all places for us. 78 places in total.. Probably wait with fixing that until migraiton to monorepo is done
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, eslint should sort these things automatically for us. Please, enable =)
<InfoIcon color={variant} width="10px" height="10px" /> | ||
<Text ml="0.5"> | ||
{variant !== 'success' | ||
? 'Adjust to collect weekly rewards' |
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.
Should this be in i18 json?
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.
You caught me. My other components are also missing i18.
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.
Do we need this at all?
<CardHeader | ||
step={3} | ||
headingText="Collect Rewards" | ||
bodyText="Maintain your target C-Ratio to collect your weekly rewards." |
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.
+i18
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.
Some little things
I was gonna add some cypress component tests, but I think it's better to wait until we've migrated to js-monorepo. And maybe even until we removed next, since the configuration seems a bit different when using next vs raw webpack
https://staking-storybook-git-main-action-cards-synthetixio.vercel.app/?path=/story/mainactioncard--primary