Skip to content
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 p5.Element #6512

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Conversation

nickmcintyre
Copy link
Member

Addresses #6511

Changes:
I edited the inline docs for the p5.Element methods fromparent() through dragLeave() to bring them more in line with the style guide. These docs live in src/core/p5.Element.js. I'll edit the docs from src/dom/dom.js in another pull request.

@dkessner, @MsQCompSci, @davepagurek, @limzykenneth, @Qianqianye, @SarveshLimaye, @SoundaryaKoutharapu, @ramya202000, @BamaCharanChhandogi, @Obi-Engine10, @MarceloGoncalves, @hiddenenigma 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
  • [Inline documentation] is included / updated

@MsQCompSci
Copy link

These might be helpful for new users:

  • for parent() maybe adding some comments that describes how each example is different in the examples, or what specific lines of code do will make the examples easier to interpret (same for class(), mousePressed(), doubleClicked(), mouseWheel(), mouseReleased(), mouseClicked(), mouseMoved(), mouseOver(), mouseOut(), all functions that have "touch")
  • for mousePressed() and some other elements the phrase "Sets a function to call once each time the ..." is a bit confusing. "...once each time..." will confuse younger readers
  • A note for the functions that include "touch" it is unclear how touch is different from clicked or released.

@nickmcintyre nickmcintyre mentioned this pull request Nov 7, 2023
2 tasks
@dkessner
Copy link

dkessner commented Nov 8, 2023

Hi Nick, I like all the changes you made to the p5.Element methods() -- they are much simpler overall.

Two thoughts:

  1. For the 'deeppink' examples (e.g. mousePressed()), my natural inclination is to click again and I'm expecting the background color to toggle back to grey. But this would have the tradeoff of an extra variable to make the code a little more complicated.

  2. I like that there are multiple examples for mousePressed() and others. I think it may be helpful, especially for any coders new to Javascript, to have an example where the callback function is defined externally (i.e. not with =>):

function setup() {
//...
cnv.mousePressed(handleMousePressed);
}

function handleMousePressed() {
circle(mouseX, mouseY, 20);
numCircles +=1;
}

@MsQCompSci
Copy link

Awesome work!
Some more thoughts:

  1. In the description for .parent() - Visual learners will benefit from some sort of diagram or visual that shows boundaries for parent and childs divs - For example: a diagram like this one
  2. Add simple comments to describe what the code does (for example in .id() comments on a line after function setup can read: // creates canvas element in cnv, and sets id to "my canvas"; on the line before background(200) comments can read //sets background color, puts the id of the canvas element in a variable id, and displays on canvas (similar feedbak for .class(),
  3. In .class() - The first example shows you set the class name but there isn't any indication of what was done on the comments or in the output. Might be a little ambiguous for newer users.

@nickmcintyre
Copy link
Member Author

@dkessner @MsQCompSci very helpful feedback per usual, thank you! I'll incorporate your suggestions over the weekend.

@MsQCompSci
Copy link

  • in .mouseWheel() it isnt obveous what a mouse wheel is
  • the touch examples werent working for me. Maybe I was doing something wrong (also helpful information on maybe giving a prompt on how to interact with the touch examples

@nickmcintyre
Copy link
Member Author

@dkessner @MsQCompSci great suggestions–thank you!

@Qianqianye @limzykenneth @davepagurek if these edits look good to you, please go ahead and merge them.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@davepagurek davepagurek merged commit b761c95 into processing:main Nov 14, 2023
2 checks passed
@nickmcintyre nickmcintyre deleted the sod-p5-element branch May 6, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants