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

Expand on foundational JS docs #4057

Merged
merged 27 commits into from
Oct 14, 2019

Conversation

stalgiag
Copy link
Contributor

@stalgiag stalgiag commented Oct 4, 2019

This PR builds on the great work by @singhvisha .

I wanted to try my hand at adding some additional beginner-friendly language to describe some of the key JS basics and supplement the linked MDN pages. This requires trying to figure out how much information to give and how much to withhold.
As expected, this is a really difficult needle to thread.

So any and all help with making the descriptions more clear and concise is welcome.
In addition, any higher-level edit suggestions are welcome. This can be anything from formatting to a suggestion to omit or include a specific reference.

Also, I have a couple more specific areas that I welcome opinions on:

  • My instinct is to omit the for-in and for-of references. I am finding it difficult to explain the distinction between them in beginner-friendly language. I also feel that these two are a little on-the-edge of my imagined boundaries for these docs.
  • Also, feel that it might be good to omit parseInt(). We have int() in the library.
  • I don't use any of the p5 functions in the examples. This makes sense to me since we are showing code that should work outside of setup() etc. But it means a ton of instances of console.log. (which might be better anyways since print() is contested)
  • I included the types even though this is a murky area with JS. I think it is useful for beginners to feel comfortable with these terms. It also helps with cross-referencing when describing what other things are for. (ie classes referencing objects)

I think it is important to nail down the scope of the references and language used within them. So this means I will probably keep passing over these for a bit till they are solid.

Any and all input is welcome, especially from people that have experience teaching these concepts.

Gonna no-pressure tag a few people
@shubymao @singhvisha @lmccart @outofambit @brysonian

closes #3897

@lmccart
Copy link
Member

lmccart commented Oct 5, 2019

this is great @stalgiag and thanks @singhvisha for the work on this too!
I agree on all the points you mention in your comment, I will give the code a closer look over in the next couple days.

Copy link
Member

@lmccart lmccart left a comment

Choose a reason for hiding this comment

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

looking great. a few notes inline.
one other thought -- I think it would be good to include console.log. we can mention it's equivalent (basically) to print(). especially if some of these examples make use of it.

src/core/reference.js Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Show resolved Hide resolved
Copy link
Member

@limzykenneth limzykenneth left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few things i've noticed.

src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
src/core/reference.js Outdated Show resolved Hide resolved
@brysonian
Copy link
Contributor

@singhvisha and @stalgiag this is great!

@stalgiag
Copy link
Contributor Author

stalgiag commented Oct 7, 2019

Thank you @limzykenneth , @lmccart , and @brysonian !

I think I covered all of the requests. I still need to do wording and clarity passes but this is starting to feel good.

@stalgiag
Copy link
Contributor Author

stalgiag commented Oct 12, 2019

I added the comparison operators as per @brysonian 's suggestion.

The tests above will fail because the linter doesn't like the example with the double ==. I don't think it is possible to disable a linting rule within a sample with our current sample linting system.

If you do /* eslint-disable eqeqeq */at the top of the file, this will not apply to the sample as the rule-disabling-line isn't extracted by the sample-linter.

So the few remaining questions:

  1. Anybody have a lead on how to disable a linting rule for a single example? My instinct is that it will require modifying sample-linter.js.
  2. Should I also make a reference for ++, --, &&, ||? These are pretty central, but it is also starting to feel a bit crowded.
  3. Both JSON.stringify() and console.log() display incorrectly in the syntax section. See pic:
    Screenshot (143)
    This is inline with how all of the other class member methods in the library are displaying, but these two are different in that they are typically going to be used with the respective global window property. Because of this it is a bit confusing to see the syntax as log().

@brysonian
Copy link
Contributor

does == need to be in there? If the only place it appears in the samples is here then maybe it isn't necessary. There are so few instances where one would use it anyway.

@limzykenneth
Copy link
Member

I think it's important to include == not because it is useful or recommended (though I would say useful if you don't want to care about types) but because there are many examples past, present, and probably future, that are still using it so it is useful to keep it around at least as a contrast to the behaviour of === and potential confusion that may arise for people coming from other programming languages.

@stalgiag
Copy link
Contributor Author

Yeah similar to what @limzykenneth is saying, I thought it useful to have == just for contrast against ===. I think the distinction between these two can be a common snag for beginners and people transitioning from strongly-typed languages.

That said, I wonder if perhaps == could just be a footnote in the description for ===. Do you think that would suffice @limzykenneth ?

@limzykenneth
Copy link
Member

Yeah, I think just some mention of == in === would be sufficient.

@stalgiag
Copy link
Contributor Author

Okay, == hangup resolved.

Quoting myself just to bump the still relevant questions down the discussion chain.

  1. Should I also make a reference for ++, --, &&, ||, !==? These are pretty central, but it is also starting to feel a bit crowded.
  2. Both JSON.stringify() and console.log() display incorrectly in the syntax section. See pic:
    Screenshot (143)
    This is inline with how all of the other class member methods in the library are displaying, but these two are different in that they are typically going to be used with the respective global window property. Because of this it is a bit confusing to see the syntax as log().

@lmccart
Copy link
Member

lmccart commented Oct 13, 2019

@stalgiag as a cheap hack, if you write it as window.console.log() does it display as desired?

@stalgiag
Copy link
Contributor Author

@lmccart this relates to a problem we encountered with the first pass on the docs. Basically a dot can't go in any of the fields.

But in good news, I just figured out that I can add the @static property as a hack and we get the desired text in the syntax field. Will push changes in a minute!

@stalgiag
Copy link
Contributor Author

Okay, this feels ready for final review and then merging.

Those other references (++, --, &&, ||, !==) can potentially become another issue. I don't want to make this PR too large and it seems unwieldy to capture all of the references that might be helpful in one go.

@stalgiag stalgiag removed the WIP label Oct 13, 2019
@lmccart lmccart merged commit f5556e5 into processing:master Oct 14, 2019
@lmccart
Copy link
Member

lmccart commented Oct 14, 2019

🎉 thanks @stalgiag and everyone else, this is fantastic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

p5js Reference should include entries for JavaScript basics
4 participants