Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Add option definitions and validation to Graph3d #3099

Merged
merged 20 commits into from
Jul 23, 2017

Conversation

wimrijnders
Copy link
Contributor

This finally adds the options checking to graph3d. I felt the need to do this now, because of expected changes in the options in the near future. Better to have the validation enabled for that.

  • Added option definitions to graph3d
  • Added unit test for checking the validity of graph3d default options
  • Commented out 'autoByDefault' from default options - module Validator can't handle explicit setting of value to undefined. The comments now serve as internal documentation
  • Checked all graph3d examples for correct options, modified these where necessary (notably playground)
  • Added onclick to documentation

@yotamberk
Copy link
Contributor

One question: Why do you need to remove options that are undefined? Does the Graph3D not accept undefined options? Does it error?

@wimrijnders
Copy link
Contributor Author

wimrijnders commented May 25, 2017

Validator chokes on undefined. I've been looking into the code but I can't fix it.
It doesn't understand the difference between a variable that's not there and a variable that is explicitly set to undefined. I gave up on it.


Update: I have to add, 'undefined' has special meaning in graph3d. It means 'calculate this option from the incoming data'. It's kind of hard to completely consolidate this with the working of Validator.

@yotamberk
Copy link
Contributor

I don't love it, but I can accept it for now. It's definitely a TODO.

@wimrijnders
Copy link
Contributor Author

Yeah, I agree. But I haven't found a fix for it (yet). Maybe if my head is clearer.

@wimrijnders
Copy link
Contributor Author

Unit test fails now. I'll also see if I can do something about the undefined handling.

@wimrijnders
Copy link
Contributor Author

OK, I figured out why the unit tests didn't work. The problem was that I was an idiot. I'm not sure how to fix that, I hope it's just a glitch.

Also, explicit undefined is now handled properly in the Graph3d options. Removed the silly code in the examples (2 instances) that removed explicitly undefined options.

@yotamberk
Copy link
Contributor

@wimrijnders conflict to be resolved =)

@wimrijnders
Copy link
Contributor Author

@yotamberk conflict fixed.

@@ -16,10 +16,10 @@
"complexity": [2, 55],
"max-statements": [2, 115],
"no-unreachable": 1,
"no-useless-escape": 0,
"no-console": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want this in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for now. console is used in the running code to output warning messages.

But this is from a practical viewpoint. I would be happiest if as many checks as possible are enabled for eslint. It's just that console is used a bit too often to casually fix this.

@yotamberk yotamberk merged commit 536d250 into almende:develop Jul 23, 2017
@wimrijnders
Copy link
Contributor Author

🎉 Yay progress! All Graph3d PR's are now merged, which means I can advance it again. But first, bugfixes 😞.

@wimrijnders wimrijnders deleted the optionsGraph3d branch July 24, 2017 05:30
primozs pushed a commit to primozs/vis that referenced this pull request Jan 3, 2019
* Proof of concept with copied options + handling from network

* Added unit test for Graph3d, for checking default syntax; completed def's of all options, autoByDefault not handled yet.

* Fixes for options in playground example

* Added onclick options to graph3d documentation

* Fixes in graph3d examples

* Final fixes for option definitions in Graph3d

* Fixed handling of 'undefined' in options, enhanced graph3d unit test

* Disabled console output in graph3d unit test

* Upgrade webpack module
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants