-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
docs/httpPost #2163
docs/httpPost #2163
Conversation
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
* createCanvas(800, 800); | ||
* } | ||
* | ||
* function mousePressed() { |
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.
Should I add a note about p5.dom ?
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.
This form of mousePressed
is bundled with p5.js, only p5.Element.mousePressed
is bundled with p5.dom.js so the user probably won't need p5.dom.js when running this example on their own.
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.
Cool - new here. Thanks!
src/io/files.js
Outdated
* <div><code> | ||
* // Examples use jsonplaceholder.typicode.com for a Mock Data API | ||
* | ||
* var url = 'https://jsonplaceholder.typicode.com/posts'; |
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 assume this service doesn't have rate limiting?
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 don't think so. Can't find any documentation and my own testing seems to agree
src/io/files.js
Outdated
* ellipse(mouseX, mouseY, 200, 200); | ||
* text(result.body, mouseX, mouseY); | ||
* }, | ||
* function (error) { |
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 the error callback can be omitted to show it's optional and I think it may only be used in some cases.
Edit: Since you already have example showing using the error callback, maybe omit them in this example?
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.
Sure, will do!
src/io/files.js
Outdated
* // Examples use jsonplaceholder.typicode.com for a Mock Data API | ||
* | ||
* var url = 'https://jsonplaceholder.typicode.com/posts'; | ||
* var post = { userId: 1, title: 'p5 Clicked!', body: 'p5.js is way cool.' }; |
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 would prefer another name for the variable just so to not be confused with httpDo
's "method" argument
src/io/files.js
Outdated
* // ... won't be called | ||
* }, | ||
* function (error) { | ||
* console.error(error); |
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.
Maybe remove this console.error
, the error callback should be a mechanism to catch error and not throw them. (Avoiding exception that will be thrown by default if no error callback is provided)
Thanks @austince! Overall just have a look at the line comments I left. Other than that, it would be nicer to have something that do more than just printing the returned data on screen but this serves as a good start. |
Sounds good, I can get more creative with it, just wanted to get general feedback on the format before I got too deep. |
Signed-off-by: austin <[email protected]>
Signed-off-by: austin <[email protected]>
src/io/files.js
Outdated
* text(result.body, mouseX, mouseY); | ||
* }); | ||
* | ||
* // you can also omit the datatype and httpPost will guess |
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 we can omit this block.. it's useful to know but makes the example look longer than it actually is. Maybe instead, we just add your note to the [datatype] param definition line above
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 great! one small change then it's ready to merge. |
Signed-off-by: austin <[email protected]>
Cool, thanks! 💯 |
awesome thank you! 🎉 |
Working on documentation from this issue: #1954
Let me know if this format looks good and I'll keep doin' it!