-
-
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
#6006 issue: Update p5.Color.js #6047
#6006 issue: Update p5.Color.js #6047
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.
Thanks for making this! Everything looks pretty good, I just have two minor comments. Let me know if I can clarify anything!
src/color/p5.Color.js
Outdated
@@ -24,9 +24,17 @@ import color_conversion from './color_conversion'; | |||
* We also cache normalized, floating-point components of the color in various | |||
* representations as they are calculated. This is done to prevent repeating a | |||
* conversion that has already been performed. | |||
* | |||
* <a href="#/p5/color">color()</a> is the recommended way to create an instance | |||
* of this class. But, if we use constructor then, it is required to use parameters. |
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.
It might be slightly clearer if, instead of "But, if we use constructor then, it is required to use parameters," we instead something like, "However, one can also create a color instace from the constructor using the parameters 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.
Yeah, good suggestion
src/color/p5.Color.js
Outdated
* | ||
* @class p5.Color | ||
* @constructor | ||
* @param {p5} [pInst] pointer to p5 instance. | ||
* | ||
* @param {p5.color|Number[]|String} vals an array or object containing the color |
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.
It looks like the _parseInputs
function in the constructor doesn't handle a p5.Color
, so I think the type here can just be Number[]|String
. We can then also take out the "or object" part from the description.
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.
Okay, I will correct it.
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 making the changes, looks great!
Looks like the linting test failed because there are trailing spaces, I'll just remove those |
@all-contributors please add @sawaisinghh for docs |
This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token , in JSON at position 100743 |
@all-contributors please add @sawaisinghh for documentation |
This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 101008 |
@all-contributors please add @sawaisinghh for documentation |
I've put up a pull request to add @sawaisinghh! 🎉 |
(sorry for the noise! had to fix some json issues before it'd work.) |
Resolves #6006
Changes:
Document parameters of p5.Color constructor
PR Checklist
npm run lint
passes