Skip to content
This repository was archived by the owner on Oct 18, 2023. It is now read-only.

Conversation

@barthuijgen
Copy link

@barthuijgen barthuijgen commented Jul 24, 2023

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.

@netlify
Copy link

netlify bot commented Jul 24, 2023

Deploy Preview for festive-swartz-adea72 ready!

Name Link
🔨 Latest commit 2fed5e9
🔍 Latest deploy log https://app.netlify.com/sites/festive-swartz-adea72/deploys/64be52150c656a00087779ec
😎 Deploy Preview https://deploy-preview-102--festive-swartz-adea72.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@webpro webpro left a 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");
Copy link
Contributor

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.

Copy link
Author

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 👍

```shell
npm run start
pnpm run dev
Copy link
Contributor

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.

Copy link
Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants