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

feat: add transaction components #787

Merged
merged 23 commits into from
Jul 16, 2024
Merged

feat: add transaction components #787

merged 23 commits into from
Jul 16, 2024

Conversation

abcrane123
Copy link
Contributor

@abcrane123 abcrane123 commented Jul 12, 2024

What changed? Why?

  • add TransactionButton, TransactionGasFee and TransactionStatus components

Notes to reviewers

How has it been tested?

}: TransactionReact) {
return (
<TransactionProvider address={address} contracts={contracts}>
<div className={className}>{children}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

setErrorMessage,
setIsLoading,
transactionId,
setTransactionId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep alphabetical order

if (transactionId) {
return (
<div className={containerClassName} data-testid={testID}>
<p>Successful!</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

So it's interesting how we want to build this because, we want to have two groups

<Transaction
  address={accountAddress}
  ​​className="custom-style"
  contracts={contractsToCall}
  onError={handleTransactionError}
  onStatusChange={handleTransactionStatusChange}
  sponsorOptions={sponsorOptions}
>
  <TransactionButton text={optionalButtonText} />
  <TransactionGasFee>
    <TransactionGasFeeLabel />
    <TransactionGasFeeEstimated />
    <TransactionGasFeeSponsoredBy />
  </TransactionGasFee>
  <TransactionStatus>
    <TransactionStatusLabel />
    <TransactionStatusAction />
  </TransactionStatus>
</Transaction>

We want

<TransactionGasFee>
    <TransactionGasFeeLabel />
    <TransactionGasFeeEstimated />
    <TransactionGasFeeSponsoredBy />
  </TransactionGasFee>

to be used for the initial flow, and than

 <TransactionStatus>
    <TransactionStatusLabel />
    <TransactionStatusAction />
  </TransactionStatus>

for the second part of the flow.

And if you see the sub components will get the information and we can customize their style.

import type {
TransactionContextType,
TransactionProviderReact,
} from '../types';
Copy link
Contributor

Choose a reason for hiding this comment

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

types always at the end

@@ -21,9 +42,52 @@ export type TransactionErrorState = {
TransactionError?: TransactionError;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

nit. remember to keep everything alphabetically ordered.

const { errorMessage, isLoading, transactionId } = useTransactionContext();

// TODO: rethink ordering and add additional cases
const label = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can create an utility function, similar to getSwapMessage, to help organize those cases.

[baseSepolia.id]: http(),
},
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, were you able to get this Story working, because I was trying build some complex story with different Wagmi providers, and they were not quite working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was able to get it working for TransactionButton but i tried to create a more complex one for the Transaction component and haven't been able to figure that one out yet which is why i created the docs pages

Copy link
Contributor

Choose a reason for hiding this comment

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

All good, I asked Sid to help us out with Storybook in the future.

address: '0xFBA3912Ca04dd458c843e2EE08967fC04f3579c2',
abi: erc20Abi,
functionName: 'approve',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I think about it, if we want to test a contract in our docs, probably that should be something super chill on Base Sepolia. Something to think about it on Monday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah agreed going to ask in the group for help on this

import App from '../../components/App';

# `<Transaction />`

Copy link
Contributor

Choose a reason for hiding this comment

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

Add the

Component is actively in development. Stay tuned for upcoming releases.

if (transactionId) {
label = 'Successful!';
actionElement = (
<a href={`https://basescan.org/tx/${transactionId}`} target="_blank">
Copy link
Contributor

Choose a reason for hiding this comment

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

to remember href will change based on the Chain.

We can create an utility called getChainExplorer, and have it at top level similar on how we have isBase, and that can be an utility to use across all modules.

label = 'Something went wrong. Please try again.';
labelClassName = color.error;
actionElement = (
<button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for future us, we need to get on top of the Accessibility story. @fakepixels at some point we should do an Accessibility week work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@siddharthkul would love to hear from you how Storybook can help us catch and improve those.

@Zizzamia Zizzamia marked this pull request as ready for review July 16, 2024 23:21
@Zizzamia Zizzamia merged commit 816abce into main Jul 16, 2024
7 of 11 checks passed
@Zizzamia Zizzamia deleted the alissa.crane/transaction branch July 16, 2024 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Development

Successfully merging this pull request may close these issues.

2 participants