-
Notifications
You must be signed in to change notification settings - Fork 165
solves docs issue 837 for p5.js 2.x #879
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
base: 2.0
Are you sure you want to change the base?
Conversation
Thanks for working on this! Could you add "addresses #837" to the body of the PR please? This way, it can be linked to the related issue. Thank you! |
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.
In line number 17,
In this guide, we'll explore the fusion of p5.js and Node.js
The link of node.js is not working since you're using http://node.js/
but actually the link for that is https://nodejs.org/.
Also, in many places, we mention terms like Node.js but don’t link them to their official websites. For example, when we write “Node.js,” it’s not clickable. We can improve this by replacing such mentions with [Node.js](http://nodejs.org/)
so users can directly access the official documentation.
{ files: [ | ||
"C Major Scale.json", | ||
"Frere Jacques.json", | ||
"Mary's Lamb.json" | ||
] } |
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.
Was there any reason to change the output? Do you think the output will be the same i.e.
Object i
files: Array(3)
0: "C Major Scale.json"
1: "Frere Jacques.json"
2: "Mary's Lamb.json"
length: 3
// ...prototype
Let me know what you think?
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.
Using loadJSON
is simply more idiomatic for p5.js sketches. No change to the output format.
Still if you feel something is wrong do let know
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.
Aah, correct. I think using loadJSON
would be simple and more standard, so we should switch to that.
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.
In line number 10, where we are using preload as our reference, since preload is no more defined we can probably use - en/p5/async_await
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.
Also, you should rename the getting-started-with-nodejs.mdx.todo
to getting-started-with-nodejs.mdx
to make sure it's visible on the tutorial page.
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.
Thankyou so much @NalinDalal and really sorry for the delay on working on this. I'll be reviewing this PR, feel free to tag me and ask any questions if needed. Thanks :)
updated the code as per your suggestion, however if anything goes wrong or you feel wrong, do let know @perminder-17 |
Hi, I am sorry, I still see the old code only, the code is still the same and it doesn't follow any of the suggestions given. Can you have a look once again? |
Hi thanks, now the changes are visible. Just last bits to change then we are good to go, In line number 17,
The link of node.js is not working since you're using http://node.js/ but actually the link for that is https://nodejs.org/. Also, in many places, we mention terms like Node.js but don’t link them to their official websites. For example, when we write “Node.js,” it’s not clickable. We can improve this by replacing such mentions with Node.js so users can directly access the official documentation. |
Also, I was still not clear with this change, Before it was written something like,
Now it's this after your changes. Can you explain why you changed this, what was exactly in the console that you removed the odering? |
hey @perminder-17 I will be honest with you, i got this context after going through the codebase, i felt using json was making more, however, i will change it to original, rest for link of nodejs i have updated it The output format in the console was just made simpler for the tutorial. i still am trying to reproduce the data type as result, is it okay to contact you on discord? |
Actual Behavior
The “Getting Started with Node.js” tutorial is still using p5.js 1.x patterns (e.g. preload(), old APIs, external sketches locked to 1.x). Legacy p5.js 1.x APIs (e.g. old shape-drawing signatures) remain in the code samples. Several external Web Editor sketches are locked to p5.js 1.x and won’t run under 2.x.
Expected Behavior
Since p5.js 2.0 has removed preload() and other legacy methods in favor of async/await and updated several APIs, we need to refresh this tutorial so it only uses supported 2.x functionality. Every linked sketch should has been forked (or updated) to use p5.js 2.x so examples execute correctly.
Hence I updated the docs for the required 2.x version
I have tested, linted and ran the code locally to ensure no breakage occurs
addresses pull request: #877
issue: #837