feat: add base Liquidity Hub page and types#5680
Conversation
🦋 Changeset detectedLatest commit: b8fd8b3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
147333b to
b8fd8b3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 147333b7e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return ( | ||
| <Card css={styles.getRoot({ breakpoint })} {...otherProps}> | ||
| <Card | ||
| className={cn('p-0', !isBreakpointUp && 'bg-transparent border-0', className)} |
There was a problem hiding this comment.
Preserve table chrome when no breakpoint is set
For Table callers that intentionally omit breakpoint (for example the desktop-only E-mode group table), isBreakpointUp is always false because it is gated by !!breakpoint, so this class now strips the Card background and border at every viewport size. The previous CSS only made the table transparent below an explicit breakpoint; guard this branch with breakpoint so no-breakpoint tables keep their normal Card styling.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR scaffolds the Liquidity Hub feature: a new testnet-only page showing a table of hub vaults with supply/APY/liquidity stats, backed by mock data (explicitly TODO'd) and gated behind a
Confidence Score: 3/5Safe to merge once the OperationModal fix is in place; the entire feature is testnet-only and behind a flag, so there is no production user impact today. The apps/evm/src/pages/LiquidityHub/LiquidityHubTable/RowControl/OperationModal/index.tsx — destructure Important Files Changed
Reviews (1): Last reviewed commit: "feat: add base Liquidity Hub page and ty..." | Re-trigger Greptile |
| export const OperationModal: React.FC<OperationModalProps> = ({ ...otherProps }) => ( | ||
| <Modal isOpen {...otherProps}> | ||
| {/* TODO: add content */} | ||
| <div>Add content here</div> | ||
| </Modal> | ||
| ); |
There was a problem hiding this comment.
liquidityHub prop leaked into <Modal> via spread
OperationModalProps adds liquidityHub: LiquidityHub on top of Omit<ModalProps, 'children' | 'isOpen'>. Because the component uses { ...otherProps } without extracting liquidityHub, the prop flows through to <Modal> and then down to MUIModal. ModalProps does not include liquidityHub, so TypeScript's excess property check on the JSX spread will fail at yarn tsc. Even if somehow it compiled, React would warn about an unknown attribute on the underlying DOM element.
Fix: destructure liquidityHub out before spreading the rest into <Modal>.
| }} | ||
| /> | ||
| ), | ||
| selectOptionLabel: t('liquidityHub.table.columns.exposure.title'), |
There was a problem hiding this comment.
Exposure
selectOptionLabel contains raw <InfoIcon/> markup
t('liquidityHub.table.columns.exposure.title') returns the literal string "Exposure <InfoIcon/>" because t() does not process component interpolations like Trans does. In the mobile sort dropdown, users would see Exposure <InfoIcon/> as plain text.
A dedicated plain-text key (e.g., liquidityHub.table.columns.exposure.label: "Exposure") should be added to the translations and used for selectOptionLabel instead.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Jira ticket(s)
VPD-1559
Changes