Skip to content
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

[Withdraw CoW AMM]: Create withdraw pool hook UI #9

Open
wants to merge 5 commits into
base: jean/cow-346-airdrop-hook-ui-refactoring-based-on-designs
Choose a base branch
from

Conversation

yvesfracari
Copy link

@yvesfracari yvesfracari commented Sep 17, 2024

Screenshare.-.2024-09-17.4_30_16.PM.mp4

Copy link

linear bot commented Sep 17, 2024

…ns' into pedro/cow-370-create-withdraw-pool-hook-ui-2
@yvesfracari yvesfracari changed the title Pedro/cow 370 create withdraw pool hook UI 2 [Withdraw CoW AMM]: Create withdraw pool hook UI Sep 17, 2024
Comment on lines +43 to +45
items={connectedChainAirdrops.map((airdrop) => {
return { value: airdrop.name, id: airdrop.name }
})}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#would wrap this in useMemo

Comment on lines +46 to +48
setSelectedItem={({ id }) =>
setSelectedAirdrop(connectedChainAirdrops.find((airdrop) => airdrop.name === id))
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#would wrap this in useCallback

return (
<>
<p>
Reduce or withdraw liquidity from a pool before a token swap integrating the process{' '}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is a CoW AMM pool, not any AMM pool right? would just indicate it in the copy

Comment on lines +9 to +11
token: new Token(1115511, '0x912ce59144191c1204e64559fe8253a0e49e6548', 18, 'ARB', 'Arbitrum'),
fiatAmount: '2765252315984615',
balance: '100000000000000000',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 maybe I don't have context here, but I assume we hardcoding this data here for testing purposes, right? Should we add a comment to come back to this in the future and remove it?

import { IMinimalPool } from '../types'

export function useUserPools(user?: string): IMinimalPool[] {
// TODO: replace with real data

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +44 to +46
items={pools.map((pool) => ({ value: pool.symbol, id: pool.id }))}
text={selectedPool?.symbol || 'Choose a pool to remove liquidity from..'}
setSelectedItem={({ id }) => setSelectedPool(pools.find((pool) => pool.id === id))}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, would use useMemo/useCb combo here

Comment on lines +12 to 13
[SupportedChainId.SEPOLIA]: [PRE_BUILD, AIRDROP_HOOK_APP, PRE_WITHDRAW_COW_AMM],
[SupportedChainId.SEPOLIA]: [PRE_BUILD, AIRDROP_HOOK_APP],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct? We have two Sepolia keys here

display: inline-block;
border-radius: 50%;
line-height: 0;
box-shadow: 0 2px 10px 0 ${({ theme }) => (theme.darkMode ? '#496e9f' : '#bfd6f7')};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are colors usually hard coded in this project?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loved this picture 😆

Copy link

@luizmuraro luizmuraro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work and very beautiful component! Just pointed some small things. 🚀

Comment on lines +33 to +35
{poolBalance.map((poolBalance, index) => (
<PoolBalancePreview key={index} id={`${label}-${index}`} poolBalance={poolBalance} />
))}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should avoid using the index as key, in many cases it can cause a lot of trouble.
https://robinpokorny.com/blog/index-as-a-key-is-an-anti-pattern/

Comment on lines +33 to +35
text-align: center;
display: flex;
flex-direction: row;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but I think that flex's direction on React it's row by default.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ types file

justify-content: space-between;
cursor: pointer;
background-color: inherit;
padding: 0.5rem;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, there's some reason to use rem here and px on other places?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants