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

Add example page for Next app router and DaisyUI #26

Merged
merged 12 commits into from
Jan 23, 2024
Merged

Conversation

Maxtho8
Copy link
Contributor

@Maxtho8 Maxtho8 commented Jan 10, 2024

This PR, inspired by #4.
Two examples pages are added using Next.js app router and DaisyUI with tailwind :

Send Ether transaction
Send ERC20 transaction

react-toastify is added

Copy link

vercel bot commented Jan 10, 2024

@Maxtho8 is attempting to deploy a commit to the useWeb3 Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Jan 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nexth ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2024 7:49am

}, [txSuccess, txError])

const formatBalance = (balance: string) => {
return Number.parseFloat(Number.parseFloat(balance).toFixed(2)).toExponential()
Copy link
Owner

Choose a reason for hiding this comment

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

Your balance comes in wei (1e18). You probably want to convert this different.
e.g. Viem offers utils to parse/format units. See https://viem.sh/docs/utilities/formatEther

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, wagmi returns the balance in Ether as a floating point number (the response is a string), for example, '0.123456789'. How do you prefer that I format the balance?

Copy link
Owner

Choose a reason for hiding this comment

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

I think your input param shouldn't be of type string, but a bigint
See https://wagmi.sh/react/api/hooks/useBalance#data

Values are typically denominated in wei (1e18 decimals) and you'd format using the util functions I shared before.

This is how it currently looks like (I have a balance of 0.585 ETH)
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I made changes, what do you think ?

Copy link
Owner

Choose a reason for hiding this comment

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

I agree a local/hardhat chains might be useful, but I don't think it should be added by default for all builds.
e.g. only add if process.env.NODE_ENV !== 'production'

Copy link
Contributor Author

@Maxtho8 Maxtho8 Jan 21, 2024

Choose a reason for hiding this comment

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

export const ETH_CHAINS = [
  mainnet,
  sepolia,
  polygon,
  optimism,
  arbitrum,
  process.env.NODE_ENV != 'production' ? hardhat : undefined,
]

Like this ?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it would technically work, as you'd never select it, but you are adding a undefined item to the array. It would be better to e.g.

let chains = [ mainnet, sepolia, polygon, optimism, arbitrum ]
if (process.env.NODE_ENV !== 'production') chains.push(hardhat)

export const ETH_CHAINS = chains

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey I understand

@@ -15,6 +16,7 @@ export default function RootLayout(props: PropsWithChildren) {
<html lang='en'>
<body>
<Web3Provider>
<ToastContainer />
Copy link
Owner

Choose a reason for hiding this comment

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

It would be better to create a dedicated 'ToastProvider'.
That allows you to abstract the module away and standardize default functions there.

E.g. import react-toastify and the css at the provider (not in the components)

import { toast } from 'react-toastify'
import 'react-toastify/dist/ReactToastify.css'

And expose a default toast() function from the provider which calls the react-toastify toast.
This makes it easier for future upgrades, or changing the module without going through all the components that use/import it.

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 understand, I created a context for toast, you can check it here

@wslyvh
Copy link
Owner

wslyvh commented Jan 20, 2024

Hi @Maxtho8 this is great! Thanks you for contributing.
I've left some comments on some of the files. Hope it makes sense.

Also keep in mind that I've upgrading wagmi/viem and Web3Modal to their next major version.
This might result in some merge conflicts in your PR.
Not sure if it's feasible to merge, or maybe just submit a new one.

Happy to chat if you need anything

@Maxtho8
Copy link
Contributor Author

Maxtho8 commented Jan 21, 2024

Hi @wslyvh! Thank you for your feedback.

Yes, your comments make sense. I understand the utility of not embedding react-toastify in every file but rather in a provider. I hope my fix is better. However, I didn't fully understand the part about the format of balances.

I successfully merged wagmi V2 without any issues, my PR is up to date. I also fixed the example pages to align with the recommendations of wagmi V2.

@wslyvh
Copy link
Owner

wslyvh commented Jan 22, 2024

Great to hear wagmi v2 merged successfully and thanks for the updates!
The changes on the toast provider are great!

Only one minor issue with balance formatting. Let me know if you need more context

@wslyvh
Copy link
Owner

wslyvh commented Jan 23, 2024

Cool, this looks good! Thanks for the updates

2 more small nitpickings..
I'd round the balance at ~4 decimals (e.g. 0.0001 ETH). It currently shows the full balance, which can be quite long with the full 18 decimals.
image

and 2) there's a type in one of the toasts: sucessful => successful

@Maxtho8
Copy link
Contributor Author

Maxtho8 commented Jan 23, 2024

Hi,
Here is the fix to round the balance :)

@wslyvh wslyvh merged commit cbd4a53 into wslyvh:main Jan 23, 2024
@wslyvh
Copy link
Owner

wslyvh commented Jan 23, 2024

Awesome! Thanks again for the PR, nice work 🙏

@wslyvh wslyvh mentioned this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants