-
Notifications
You must be signed in to change notification settings - Fork 36
feat: update voting dApp documentation to reflect new js client api #102
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for festive-swartz-adea72 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Nice first PR, but please omit the quoting changes so we can better see what's going on :)
const { PactCommand } = require('@kadena/client'); | ||
const { createExp } = require('@kadena/pactjs'); | ||
const { PactCommand } = require("@kadena/client"); | ||
const { createExp } = require("@kadena/pactjs"); |
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.
When changing the quoting it makes for a hard to review PR. In our JS code we use single quotes, so it makes sense to keep it that way. Feel free to suggest and change this convention, but please don't mix that up in a single PR.
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.
Yeah, that's a good suggestion! I'm used to just running prettier, will revert this 👍
@@ -1036,7 +1047,7 @@ Lastly, to get the result of a transaction we are using the `pollTransactions` h | |||
To run the frontend dApp, go to the frontend folder and run: | |||
|
|||
```shell | |||
npm run start | |||
pnpm run dev |
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.
People should be free to use any package manager of choice, so I guess it's best to use what everyone already knows in generic community documentation. I.e. users of Yarn, pnpm, bun etc. etc. know how to translate npm commands.
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.
Of course, but in this case we provide an example repository with code and have to make a choice there. This documentation just reflects that. It's not meant to tell people what to use themselves.
npm run dev
would also work in a pnpm workspace though, so this could be changed.
I've ran into several issues following the voting dApp guide, and some of them related to the javascript SDK, this turned out to be easier to solve using the upcoming newer version. This MR makes use of
@kadena/[email protected]
and should not be merged until that release is stable.It is accompanied by an MR on the
kadena-community/voting-dapp
repository (link), which also switched to the new Kadena client and updated the frontend project to be more modern.With these changes it should be possible again to follow along and run the code successfully without running into errors.