-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat(cosmos): Add events to cosmos #142
Conversation
JFrankfurt
commented
Aug 12, 2022
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Style updates for page symmetry
|
||
import { DAI, USDC_MAINNET } from '../constants/tokens' | ||
import useOption from './useOption' | ||
import useProvider, { INFURA_NETWORK_URLS } from './useProvider' | ||
|
||
const EventFeedWrapper = styled.div` |
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.
Add box-sizing: border-box
src/cosmos/Swap.fixture.tsx
Outdated
onTxSuccess={(txHash: string, data: any) => console.log('tx success:', txHash, data)} | ||
onTxFail={(error: Error, data: any) => console.log('tx fail:', error, data)} | ||
/> | ||
<Row align="baseline" justify="space-around"> |
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.
Change align
to start
src/cosmos/Swap.fixture.tsx
Outdated
onTxFail={(error: Error, data: any) => addEvent({ message: `onTxFail`, data: { ...data, error } })} | ||
/> | ||
<EventFeedWrapper> | ||
<H2>Event Feed</H2> |
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.
h2
, not H2
- only React elements are capitalized, native elements are not
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.
these are components import { H2, H3 } from 'theme/type'
I'll do a default import to make this less confusing.
src/cosmos/Swap.fixture.tsx
Outdated
{events?.map(({ message, data }, i) => ( | ||
<EventRow key={i}> | ||
<div> | ||
<H3 padding={0}> |
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.
h3
, not H3
src/cosmos/Swap.fixture.tsx
Outdated
tokenList={tokenList} | ||
width={width} | ||
routerUrl={routerUrl} | ||
onConnectWalletClick={() => |
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.
Simplify the Promise-returning event handlers as
onConnectWalletClick={() => {
addEvent({ message: 'onConnectWalletClick', data: {} })
return Promise.resolve(true)
}}
src/cosmos/Swap.fixture.tsx
Outdated
<H3 padding={0}> | ||
<Message>{message}</Message> | ||
</H3> | ||
<pre>{JSON.stringify(data, null, 2)}</pre> |
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.
Make a new Event wrapper for the JSON:
const Event = styled.pre`
margin-top: 1em;
`
src/cosmos/Swap.fixture.tsx
Outdated
/> | ||
<EventFeedWrapper> | ||
<H2>Event Feed</H2> | ||
{events.length > 0 && <button onClick={() => setEvents([])}>clear</button>} |
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.
nit: You can leave the clear button visible to reduce layout thrashing
src/cosmos/Swap.fixture.tsx
Outdated
routerUrl={routerUrl} | ||
onConnectWalletClick={() => | ||
new Promise((resolve) => { | ||
addEvent({ message: 'onConnectWalletClick', data: {} }) |
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.
If no data is passed, then data should be undefined, not {}
- otherwise it suggests that you can use property access on it.
I think that you should always just pass through the event, so that the handlers look like:
(data: unknown) => addEvent({ message: 'onTxSubmit', data })
or
(data: unknown) => {
addEvent({ message: 'onTxSubmit', data })
return Promise.resolve(true)
}
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 could also make these into two callback helpers:
const addNopEvent = useCallback((message: string) => (data: unknown) => addEvent({ message, data }), [addEvent])
const addResolvingEvent = useCallback((message: string) => (data: unknown) => {
addEvent({ message, data })
return Promise.resolve(true)
}, [addEvent])
src/cosmos/Swap.fixture.tsx
Outdated
margin: 0; | ||
` | ||
|
||
type HandlerEventMessage = { message: string; data: Record<string, any> } |
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.
nit:
interface HandlerEvent {
name: string
data: Record<string, any>
}
src/cosmos/Swap.fixture.tsx
Outdated
@@ -42,7 +46,10 @@ const Message = styled.pre` | |||
margin: 0; | |||
` | |||
|
|||
type HandlerEventMessage = { message: string; data: Record<string, any> } | |||
type HandlerEventMessage = { |
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 should be an interface, not a type (it's handled better by the checker).
<H2>Event Feed</H2> | ||
{events.length > 0 && <button onClick={() => setEvents([])}>clear</button>} | ||
<Type.H2>Event Feed</Type.H2> | ||
<button onClick={() => setEvents([])} disabled={events.length === 0}> |
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 doesn't need to be disabled - it just adds complexity.
@@ -15,7 +15,7 @@ import { useCallback, useEffect, useState } from 'react' | |||
import { useValue } from 'react-cosmos/fixture' | |||
import { Field } from 'state/swap' | |||
import styled from 'styled-components/macro' | |||
import { H2, H3 } from 'theme/type' | |||
import * as Type from 'theme/type' |
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.
I hadn't realized H2/H3 came from here - it's fine to import them directly if you want to revert that.
🎉 This PR is included in version 2.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |