-
-
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
Edit docs for environment #6505
Conversation
Some helpful things for newer users:
|
Great suggestions @MsQCompSci, I added @Qianqianye @limzykenneth @davepagurek if these changes look good to you, please go ahead and merge them. |
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! Found one more typo, and one last bit that we may or may not want to change, depends what we think is most useful for people.
src/core/environment.js
Outdated
* unless you have requested WebGL1 via `setAttributes({ version: 1 })`, and | ||
* will fall back to WebGL1 if WebGL2 is not available. | ||
* A string variable with the WebGL version in use. The value is either | ||
* `'webgl2'`, `'webgl'`, or `'p2d'`, which is the default for 2D sketches. |
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.
Do you think we should mention somewhere in the docs what p5 constants this can be? Although examples that print the version are simpler with code, I assume the most common way this will be used is by checking if webglVersion === WEBGL2
or something like that. While === 'webgl2'
also works, it's probably a little less error prone to use the constant.
src/core/environment.js
Outdated
* multiply this by pixelDensity. | ||
* A numeric variable that stores the height of the screen display. Its value | ||
* depends on the current <a href="#/p5/pixelDensity">pixelDensity()</a>. | ||
* `displayHeight()` is useful for running full-screen programs. |
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 this one is just a property and not a function:
* `displayHeight()` is useful for running full-screen programs. | |
* `displayHeight` is useful for running full-screen programs. |
Good catches and suggestions @davepagurek – just incorporated them. |
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.
The changes look good, thanks!
Addresses #6483
Changes:
I edited the inline docs for the rest of the functions in the "Environment" group to bring them more in line with the style guide.
@dkessner, @MsQCompSci, @davepagurek, @limzykenneth, @Qianqianye, @ChihYungChang, @teragramgius, @tuminzee, @Zarkv, @robin-haxx, @Gaurav-1306 I'd love to incorporate any feedback you may have! Here's a staging version of the edits in this pull request.
PR Checklist
npm run lint
passes