Polish modals, add empty states on tables #437
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
DannyDelott
force-pushed
the
danny-polish-open-shorts
branch
from
12796fd to
8ac6467
Compare
DannyDelott
changed the title
DannyDelott
force-pushed
the
danny-polish-open-shorts
branch
from
2e407aa to
72300b7
Compare
DannyDelott
changed the title
DannyDelott
force-pushed
the
danny-polish-open-shorts
branch
from
b88f791 to
9836b0b
Compare
| className={ | ||
| "daisy-badge daisy-badge-md inline-flex text-success" | ||
| } | ||
| > |
There was a problem hiding this comment.
Adding a badge to show you how much ETH this fixed yield represents:
DannyDelott
force-pushed
the
danny-polish-open-shorts
branch
from
66ab9dd to
a1f93a9
Compare
| Approve {market.baseToken.symbol} | ||
| </button> | ||
| <p className="text-center text-body"> | ||
| Please note: You can remove your liquidity at any time, however if |
There was a problem hiding this comment.
There's something about this note being on three lines that feels like it's too much text for this modal. Does this get across the point "Note: You can withdraw liquidity at any time. If you're backing open positions, you'll receive Withdrawal Shares for later redemption." It gets across the point and looks cleaner at two lines
| const formattedBaseAmountOut = | ||
| baseAmountOut !== undefined | ||
| ? `${formatBalance({ | ||
| balance: baseAmountOut, |
There was a problem hiding this comment.
The onRemoveLiquidity callback and baseSymbol are unused in this file. Unless we'll need them later we should remove.
There was a problem hiding this comment.
Removing baseSymbol but I'd like to keep onRemoveLiquidity a little longer in case we want to e.preventDefault() (to avoid closing the modal this form is rendered in) in the near term
| className="daisy-btn-primary daisy-btn" | ||
| disabled={!removeLiquidity || removeLiquidityStatus === "loading"} | ||
| onClick={(e) => { | ||
| removeLiquidity?.(); |
There was a problem hiding this comment.
Could this just be onClick={removeLiquidity} since we don't use any params
There was a problem hiding this comment.
Good catch, I think i'll just wire up onRemoveLiquidity here instead so the event arg is used.
| <h5>Approve {market.baseToken.symbol}</h5> | ||
| </button> | ||
| <p className="text-center text-body"> | ||
| Please note: When you short hy{market.baseToken.symbol} you earn the |
There was a problem hiding this comment.
It feels like we don't need these notes here. If a user is taking an action like this they will likely have taken the time to understand the result of taking out a short. Maybe the exception is LP modal where the concept of withdrawal shares requires a note.
There was a problem hiding this comment.
I think the notes are helpful since we're introducing something new to the market. It's worth highlighting the risks at the point a user is about to transact, eg: long price can fluctuate before close, shorts earn the yield source rate, and LPs might receive withdrawal shares when closing.
| </div> | ||
| <div className="flex justify-between"> | ||
| <p className="">Matures in</p> | ||
| <p className=""> |
There was a problem hiding this comment.
remove unused class names
| </p> | ||
| </div> | ||
| <div className="flex justify-between"> | ||
| <p className="">Matures in</p> |
| <SortableGridTable | ||
| headingRowClassName="grid-cols-5 text-start" | ||
| bodyRowClassName="grid-cols-5 items-center text-sm md:text-h6 even:bg-base-300/5 h-16" | ||
| bodyRowClassName="grid-cols-5 items-center even:bg-base-300/5 h-16" |
There was a problem hiding this comment.
We should convert this over to react-table for consistency. I can do this after I add LP to the transactions table.
2 participants
Related to #433, matches the simpler style for the preview card/modal: