-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Update saveFrames
documentation
#5694
Conversation
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme 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.
Just one comment inline
src/image/image.js
Outdated
@@ -420,15 +420,20 @@ p5.prototype.saveGif = function(pImg, filename) { | |||
* as an argument to the callback function as an array of objects, with the | |||
* size of array equal to the total number of frames. | |||
* | |||
* Note that <a href="#/p5.Image/saveFrames">saveFrames()</a> will only save the first 15 frames of an animation. | |||
* Note that <a href="#/p5.Image/saveFrames">saveFrames()</a> will only save the first 15 seconds of an animation. |
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 we need this line anymore with the explanations below.
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.
@jesi-rgb Still have this pending then we can merge.
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.
Got it and also a minor typo!
Seems like this is already working fine! It let me commit with the correct configuration! |
Looks good. Thanks! |
Issue
Resolves #5685, in which some discussion was made regarding the behaviour of the
saveFrames
function. @endurance21 and I were inspecting the code and thought it was not working properly, but @limzykenneth went ahead and clarified why the arguments are set up like that, but stated that there was no proper explanation for that within the docs. So that is what this change is aiming to solve!I tried to be explicit enough in my explanation for the reason to be clear, but not too technical so newcomers can properly understand the problem. Let me know if I it needs change for it to be either clearer and more explicit or less technical and more friendly!
Changes
3. Adds further explanation as to why the function works like this
5. Adds further explanation in the arguments description to make sure people know what to expect.
Screenshots of the change:
The changes I added are highlighted.
PR Checklist
npm run lint
passes