refactor: pull header & wallet components out of Swap component#123
refactor: pull header & wallet components out of Swap component#123kristiehuang wants to merge 3 commits intowallet-connect-flowfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
kristiehuang
changed the title
zzmp
left a comment
zzmp
left a comment
There was a problem hiding this comment.
This change would couple src/components/Header to src/components/Swap. Header needs to be updated so that it does not couple Swap, in case we want to use it for other widgets in the future.
| <Row gap={1}>{children}</Row> | ||
| <Row gap={0.5}> | ||
| <ThemedText.Subhead1> | ||
| <Trans>Swap</Trans> |
There was a problem hiding this comment.
Should widgets expand past Swap, this will need to be configurable.
Can you keep passing title into the header?
| export { darkTheme, defaultTheme, lightTheme } from 'theme' | ||
|
|
||
| export type SwapWidgetProps = SwapProps & WidgetProps | ||
| export type SwapWidgetProps = HeaderProps & SwapProps & WidgetProps |
There was a problem hiding this comment.
This should be an interface extending the existing props:
export interface SwapWidgetProps extends HeaderProps, SwapProps, WidgetProps {}
This is slightly easier on the type system: it reduces time for type checking and it surfaces better types downstream.
| const onSupportedNetwork = useOnSupportedNetwork() | ||
| const isDisabled = !(isActive && onSupportedNetwork) | ||
|
|
||
| const [onConnectWalletClick, setOnConnectWalletClick] = useAtom(onConnectWalletClickAtom) |
There was a problem hiding this comment.
You should be able to set this without referencing the already-set value:
const setOnConnectWalletClick = useSetAtom(onConnectWalletClickAtom)
useEffect(() => setOnConnectWalletClick(props.onConnectWalletClick), [props.onConnectWalletClick, setOnConnectWalletClick])
| </Row> | ||
| <Row gap={1}> | ||
| <Wallet disabled={props.hideConnectionUI} /> | ||
| <Settings disabled={isDisabled} /> |
There was a problem hiding this comment.
Settings is specific to the Swap widget (similar to the title) - this either needs to be a prop, or a callback to activate the Swap's settings.
3 participants
Wallet connection + swap are two separate concerns, so should be two separate components imo
Refactoring to pull wallet out of the swap component