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

Update hooks args and return values to make them more readable and add wagmi links #40

Merged
merged 18 commits into from
Nov 28, 2023

Conversation

Pabl0cks
Copy link
Collaborator

@Pabl0cks Pabl0cks commented Nov 26, 2023

This is a proposal to add more readability to the args and return values from useScaffoldContractRead, useScaffoldContractWrite and useScaffoldEventHistory hooks.

Also adding wagmi links to useScaffoldContractRead, useScaffoldContractWrite and useScaffoldEventSubscriber so our users can check configuration and types.

If you feel h2 "Parameters" is not needed in some (or all) of them, we can delete it. Also was not very sure on how to add the wagmi reference for useScaffoldEventSubscriber, so just added a small sentence mentioning it.

Closes #39

Copy link

vercel bot commented Nov 26, 2023

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

Name Status Preview Comments Updated (UTC)
scaffold-eth-2-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 28, 2023 10:20am

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

I think this looks great !! Tysm @Pabl0cks 🙌, Just moved the example description outside of Parameters at 183e59b

@Pabl0cks
Copy link
Collaborator Author

I've just pushed the Parameter tables for useScaffoldContractRead, useScaffoldContractWrite, useScaffoldEventHistory and useScaffoldEventSubscriber.

Haven't modified useDeployedContractInfo and useScaffoldContract because they are too simple, so I don't know if they would look well.

In this case I haven't add a "Default Value" column, adding the info in Description field for those parameters with default value. If you prefer to have it like in Components, let me know and I'll add it 🙌.

@technophile-04
Copy link
Collaborator

technophile-04 commented Nov 27, 2023

Tysm @Pabl0cks !!

Haven't modified useDeployedContractInfo and useScaffoldContract because they are too simple, so I don't know if they would look well.

Umm just to be consistent added them at 048b739 😅 also added some references to wagmi/viem docs


I think it almost looking perfect added a couple of comments and a small NITPICK would be :

changing "Parameters" h2 to "Configurations" since the hooks actually accept one object as parameter similar to what wagmi docs does and makes sense to me 🙌. For useDeployedContractInfo I think its fine being "Parameters" their.

And also maybe mentioning about (optional) similar to what we are doing with https://docs.scaffoldeth.io/components

@Pabl0cks
Copy link
Collaborator Author

changing "Parameters" h2 to "Configurations" since the hooks actually accept one object as parameter similar to what wagmi docs does and makes sense to me 🙌. For useDeployedContractInfo I think its fine being "Parameters" their.

And also maybe mentioning about (optional) similar to what we are doing with https://docs.scaffoldeth.io/components

  • Changed h2 for the table to "Configuration". Except for useDeployedContractInfo that keeps "Parameters".
  • Added (optional) tag to Parameter name column for those parameters that are optional.
  • Also tweaked a bit some descriptions, mostly some "The.." that were too repetitive.

TYSM @technophile-04 for your help on this PR ♥

@carletex
Copy link
Member

This is looking great!

Left a tiny thing, but this looks good for a merge. We can iterate later :)

Thanks @Pabl0cks and @technophile-04

@Pabl0cks
Copy link
Collaborator Author

Left a tiny thing, but this looks good for a merge. We can iterate later :)

Cool! Thanks for the review! I'll merge it then.

If you see something wrong in the updated useScaffoldContractRead example or Shiv detects something to be changed, I'll create a new PR for the quickfixes.

TYSM! 🙌

@Pabl0cks Pabl0cks merged commit 0a5f8a6 into main Nov 28, 2023
2 checks passed
@Pabl0cks Pabl0cks deleted the update/hooks-args-return-values branch November 28, 2023 10:31
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.

Update hooks args and return values more readable
3 participants