Skip to content

added a bunch of vector examples #2812

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

Merged
merged 4 commits into from
Apr 27, 2018
Merged

added a bunch of vector examples #2812

merged 4 commits into from
Apr 27, 2018

Conversation

aatishb
Copy link
Contributor

@aatishb aatishb commented Apr 24, 2018

Hi, I added some visual interactive examples to the p5.vector documentation. cc #1954
I'm not sure if all of this will be useful, let me know if you have any feedback on this.

@aatishb
Copy link
Contributor Author

aatishb commented Apr 24, 2018

I'm not sure why the new examples aren't passing the test. They all seem to work fine when I run grunt yui:dev or grunt test, and they don't give an error in test/test-reference.html.

This is the error message I get when I run grunt (one for each new example):

  1) src/math/p5.Vector.js
       toString documentation
         example #2 works:
     
  eval@[native code]
  http://localhost:9001/test/test-reference.html:79:15
  p5@http://localhost:9001/lib/p5.js:46725:11
  http://localhost:9001/test/test-reference.html:160:26
  initializePromise@http://localhost:9001/lib/p5.js:28230:13
  Promise$2@http://localhost:9001/lib/p5.js:28638:50
  testFunc@http://localhost:9001/test/test-reference.html:153:29
  callFn@http://localhost:9001/node_modules/mocha/mocha.js:4490:25
  run@http://localhost:9001/node_modules/mocha/mocha.js:4482:13
  runTest@http://localhost:9001/node_modules/mocha/mocha.js:4996:13
  http://localhost:9001/node_modules/mocha/mocha.js:5114:19
  next@http://localhost:9001/node_modules/mocha/mocha.js:4910:16
  http://localhost:9001/node_modules/mocha/mocha.js:4920:11
  next@http://localhost:9001/node_modules/mocha/mocha.js:4844:16
  http://localhost:9001/node_modules/mocha/mocha.js:4883:11
  done@http://localhost:9001/node_modules/mocha/mocha.js:4437:7
  callFn@http://localhost:9001/node_modules/mocha/mocha.js:4508:11
  run@http://localhost:9001/node_modules/mocha/mocha.js:4482:13
  next@http://localhost:9001/node_modules/mocha/mocha.js:4858:13
  http://localhost:9001/node_modules/mocha/mocha.js:4888:9
  timeslice@http://localhost:9001/node_modules/mocha/mocha.js:82:27

Appreciate any help.

@lmccart
Copy link
Member

lmccart commented Apr 25, 2018

hm I'd need to dig a little deeper to figure out why they're failing.. but the reference examples are meant to be really short, concise snippets of code that don't require much programming knowledge or knowledge of other p5 functions to follow. I think the drawArrow() example might fit better on the examples page. there are instructions for adding examples here: https://github.com/processing/p5.js-website/wiki/Adding-examples

@aatishb
Copy link
Contributor Author

aatishb commented Apr 25, 2018

Ah ok, I interpreted the examples page as being a place for slightly more creative applications. My goal was to provide a bit more visual intuition for each vector function. They're mostly basic uses of each function, except maybe dot() and cross()... the main reason they're long is because I added the (same) drawArrow() function to every example. I could replace that with a line if need be.
Let me know whatever makes most sense.

@@ -441,6 +703,70 @@ p5.Vector.prototype.magSq = function magSq() {
* print(p5.Vector.dot(v1, v2)); // Prints "10"
* </code>
* </div>
*
Copy link
Member

Choose a reason for hiding this comment

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

this feels like one for the examples page rather than the reference

@@ -483,6 +809,51 @@ p5.Vector.prototype.dot = function dot(x, y, z) {
* print(crossProduct);
* </code>
* </div>
*
Copy link
Member

Choose a reason for hiding this comment

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

this one might be better suited for the examples page too

@lmccart
Copy link
Member

lmccart commented Apr 26, 2018

ah I see ok. I thought that the functions you were demonstrating appeared in the drawArrow() function, which would require the user to parse through that function. but looking closer I see that they are used outside of the drawArrow() function, so this seems fine. I marked two that seem rather complex and might be better suited for the examples page, let me know what you think. the rest I agree are good to stay.

@lmccart
Copy link
Member

lmccart commented Apr 26, 2018

also try pulling the latest changes and see if the code builds and tests pass this time. @Spongman just added a fix for one common error we were seeing in this process.

@Spongman
Copy link
Contributor

I think it might be nice to move the push call to the start of the drawArrow function so the stroke & fill settings are preserved. It doesn't really matter for these examples, but if someone copies that function making that change might make it more useful generally.

@aatishb
Copy link
Contributor Author

aatishb commented Apr 26, 2018

Thanks @lmccart @Spongman for your feedback! I agree with all the suggestions, so I removed the dot() and cross() examples, and moved up the push() statement in drawArrow() for all the examples.

Once again, the examples are failing the tests with the same error I pasted above. As far as I can tell, all the examples work fine in the locally generated docs, and there are no errors introduced with them when I open /test/test-reference.html or /test/test.html. So I'm at a loss what is breaking the tests.

@aatishb
Copy link
Contributor Author

aatishb commented Apr 26, 2018

ok, after a lot of digging I figured out the problem. It seems if I replace let with var it passes the tests. So is this a bug, or a style requirement? Should I just replace let with var throughout?

@Spongman
Copy link
Contributor

good catch! yes the library isn't ready for ES6 yet.

it's weird that the linter didn't catch this. BTW the reason the test fail is that they're run in PhantomJS which uses an older version of webkit.

@lmccart lmccart merged commit b8e4f34 into processing:master Apr 27, 2018
@lmccart
Copy link
Member

lmccart commented Apr 27, 2018

aha. thanks @aatishb

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

Successfully merging this pull request may close these issues.

3 participants