-
Notifications
You must be signed in to change notification settings - Fork 192
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: SwapAmountInput #504
Conversation
site/docs/pages/swap/types.mdx
Outdated
tokenBalance?: string; // Amount of selected Token user owns | ||
onMaxButtonClick: () => void; // Callback function when max button is clicked | ||
onAmountChange: (amount: string) => void; // Callback function when the amount changes | ||
onTokenSelectorClick: () => void; // Callback function when the token selector is clicked |
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.
Maybe let's change the onTokenSelectorClick
to setToken
? The user shouldn't have to know that we are using TokenSelector under the hood.
onTokenSelectorClick: () => void; // Callback function when the token selector is clicked | |
setToken: () => void; // Callback function when the token selector is clicked |
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.
yes agreed.
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.
The other cb props i am passing in start with on i.e onAmountChange
, onMaxButtonClick
. Should I do onTokenSelect
to keep it consistent or do you think setToken
is more clear ?
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.
update: went with setToken
and setAmount
site/docs/pages/swap/types.mdx
Outdated
|
||
```ts | ||
type SwapAmountInputReact = { | ||
label: string; // Descriptive label for the input field |
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.
let's keep all types alphabetical.
a55da0b
to
508b979
Compare
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.
Great work!!!! 👏
What changed? Why?
SwapAmountInput
componentNotes to reviewers
add this back in after PR is merged:
How has it been tested?
locally