chore: remove address prop and update sponsor txn check#1114
Conversation
github-actions
bot
added
the
documentation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
abcrane123
changed the title
| ```ts | ||
| type TransactionReact = { | ||
| address: Address; // The wallet address involved in the transaction. | ||
| address?: Address; // The wallet address involved in the transaction (defaults to connected account if not provided). |
There was a problem hiding this comment.
And in the end, we decided to remove this completelly, right?
There was a problem hiding this comment.
yep just updated
abcrane123
changed the title
.changeset/brave-icons-fix.md
Outdated
| "@coinbase/onchainkit": patch | ||
| --- | ||
|
|
||
| - **patch**: Remove address prop and fix sponsor component. By @abcrane123 #1114 |
There was a problem hiding this comment.
nit. try to be more verbose and explain this as someone with no context is reading this.
| transactionId, | ||
| } = useTransactionContext(); | ||
|
|
||
| const { address } = useAccount(); |
There was a problem hiding this comment.
Should useAccount be added even in small components. Or should be synced at Provider level?
Open question
There was a problem hiding this comment.
i am leaning towards leaving this at component level because the TransactionButton component is the only component that requires address (to disable the button if address isn't defined) but let me know if you disagree
| writeContractsAsync: writeContractsAsyncMock, | ||
| }); | ||
| (useWriteContract as ReturnType<typeof vi.fn>).mockReturnValue({ | ||
| status: 'IDLE', |
There was a problem hiding this comment.
idle or IDLE?
I have a feeling it's always lowercase
There was a problem hiding this comment.
yeah you're right it is idle
|
|
||
| expect(screen.queryByText('Zero transaction fee')).not.toBeInTheDocument(); | ||
| }); | ||
| it('does not render if statusName is not init', () => { |
There was a problem hiding this comment.
My take on spaces with test is:
- no space within an
it - one space between
it
By doing that it's much easiser reading large file full of tests.
|
|
||
| expect(screen.queryByText('Zero transaction fee')).not.toBeInTheDocument(); | ||
| }); | ||
| it('does render if statusName is init', () => { |
There was a problem hiding this comment.
Usually the good practice of test, it's to start with the wording should ...
3 participants
What changed? Why?
Notes to reviewers
How has it been tested?