fix: saveGif's third argument made more optional #5931
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.
My bad 😓
I think it'll be this ⬇️ (You've mistakenly added semicolons after comments) 😅
const delay = (options && options.delay) || 0; // in seconds
const units = (options && options.units) || 'seconds'; // either 'seconds' or 'frames'Thanks for the catch @Qianqianye 💙
| @@ -238,18 +238,10 @@ p5.prototype.saveGif = async function( | |||
| if (typeof duration !== 'number') { | |||
| throw TypeError('Duration parameter must be a number'); | |||
| } | |||
| // if arguments in the options object are not correct, cancel operation | |||
| if (typeof options.delay !== 'number') { | |||
There was a problem hiding this comment.
How do you feel about putting these two checks back in, but checking delay and units rather than options.delay and options.units? That way, if one accidentally specifies units as, for example, 's' instead of 'seconds', they'll still hit an error. It will mean moving it to after we define those variables, I believe.
There was a problem hiding this comment.
Yeah, that should be good! I'm on it.
There was a problem hiding this comment.
const delay = options?.delay ?? 0
const units = options?.units ?? 'seconds';https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
😉
src/image/loading_displaying.js
Outdated
| // if arguments in the options object are not correct, cancel operation | ||
| if (typeof options.delay !== 'number') { | ||
| if (typeof delay !== 'number' && delay !== null) { |
There was a problem hiding this comment.
Looks good! Just for my understanding, was there a case that wasn't being handled that the && delay !== null is needed for?
|
Thank you both @Neilblaze and @davepagurek. I restarted the check process, and it's all passed. Ready to merge this PR if it looks good to you both. |
|
Thanks @Qianqianye! |
saveGifcurrently accepts an optional third argument in the form of an object with delay and unit keys. If an object is passed with only one of those keys defined, then it returns an error message respectively.Resolves #5927
Changes:
saveGifeven more optional.delayandunitsrather thanoptions.delayandoptions.unitsPR Checklist
npm run lintpassesThanks for the assist @davepagurek 🙌