-
Notifications
You must be signed in to change notification settings - Fork 175
Update reference generation instructions for p5.js 2.0 fork workflow #948
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
Conversation
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.
Thanks for working on this @adi-ray !
@perminder-17 I tried to test this with a fresh clone of a p5.js fork; on npm run custom:dev https://github.com/ksen0/p5.js.git#948-test
I get:
┃ Local http://localhost:4321/
┃ Network use --host to expose
15:04:48 watching for file changes...
getCurationSketches 429 Too Many Requests
getCurationSketches 429 Too Many Requests
15:04:57 [ERROR] payload2.filter is not a function
Stack trace:
at Module.eval (/Users/kuksenok/Documents/dev/ksen0/p5.js-website/src/api/OpenProcessing.ts:66:37)
at async eval (/Users/kuksenok/Documents/dev/ksen0/p5.js-website/src/layouts/HomepageLayout.astro:22:26)
[...] See full stack trace in the browser, or rerun with --verbose.
Have you encountered this in the past / do you have ideas on resolving, or will it need some more debugging/ideas? Ideally, we would suppress the OP requests for local testing (it makes sense that OP endpoint are rate-limited as they are)
@@ -380,22 +380,22 @@ Class constructors are defined with the `@class` tag and the `@constructor` tag. | |||
|
|||
The p5.js repository is set up so that you can generate and preview the reference without needing to build and run the p5.js website as well. | |||
|
|||
* The main command to generate the reference from the reference comments in the source code is to run the following command. | |||
To do so, make sure you have committed your changes to a branch of your fork of p5.js. |
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.
I think it may also be helpful to mention that this makes most sense on the 2.0
branch of the p5.js-website
repository. Also, based on the subsequent step, the changes should be committed and pushed to one's branch, right? Please ignore if I'm incorrect.
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.
I think it works fine with both branches. The only limitation is that it doesn’t work across branches.
For example, you cannot build the main branch of p5.js with the 2.0 branch of p5.js-website, and the same applies the other way around.
The main issue comes up with tutorials. For instance, the strands tutorial depends on the library from the dev-2.0 branch of p5.js, so it cannot build properly with the main branch. We could simply add a line to clarify this.
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.
@perminder-17 Absolutely makes sense - it was an unintetional error on my part, to not "match" the branches, but thanks for the explanation here and the other responses on this PR!
Hi @ksen0, sorry again for the delay. I had a look at your branch The website’s main branch should always be built with the main branch of p5.js and the website’s Let me know if the error still persists? "EDIT" : Hi @ksen0 , I just noticed the same error now. I haven't ever experienced that before. Your point makes sense, During tests and local builds, don’t call OpenProcessing at all. I think #954 fails because of that. Btw, if this errors comes while building the reference from the p5.js repo, are we able to see our website on |
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.
Hi @adi-ray can you add some images as well showing how your terminal should look like if you have done all steps correct? I think screenshots will be good to have in making new contributor understand if they have done things in correct way or not.
Hi @perminder-17 ! |
No, just for the last step when u run |
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.
Thanks for the revisions @adi-ray ! Looks great to me, please feel free to merge @perminder-17 if you think it's ready.
Because this does work both on main and on dev-2.0, once this PR is merged, please create a new PR on main
to reflect this. (Alternatively, when there is the next 1.x release, we can cherrypick your commits into main
too.)
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.
Looks good to me, thanks! I think we also need this in main
branch as well. I'll cherrypick your commits and open up a new PR in main
branch as well. Thanks for the help.
Description:
PR updates the outdated contributor documentation which was still referencing the old workflow using
npm run docs
andnpm run docs:dev
commands within the p5.js repository.Note
Please review the English content changes first. Once approved, I'll be happy to update the other language versions accordingly.
Fixes #924