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

Uniform handling of common user options #2168

Merged
merged 1 commit into from
Oct 18, 2016
Merged

Conversation

wimrijnders
Copy link
Contributor

This change removes many of the recurring lines in setOptions() of the form:

    if (options.width !== undefined)            this.width = options.width;

I hereby note that many of the rest of the lines are similar but with a prefix default, e.g.:

    if (options.xCenter !== undefined)          this.defaultXCenter = options.xCenter;

The prefix default is ultimately not required and I can remove its need in due time, which enables uniform handling also for these fields.

There are longer term reasons for this change:

First: Currently, there are two sets of options present, namely:

  • the user options, as set in setOptions()
  • the defaults, as set in the constructor of Graph3d

I believe it's for the better if these can be handled uniformly. Uniform handling of default settings is something I intend to add in the future (most likely immediately after if/when you accept this PR).

Second: I hereby remind you that my ultimate goal is to enable multiple graphs in a single view. This will add a third set of options, namely options specific to each graph present. I strongly prefer to have a uniform handling of these options as well.

What I ultimately want is for all options to be handled in the same consistent manner. This change is a first step to achieve that.

I hope you have an appreciation for this.

@mojoaxel
Copy link
Member

What I ultimately want is for all options to be handled in the same consistent manner.

I Like it. What I find very appeeling is the possibility for type checking and a clean error handling.

Maybe you could have a look at the option handling of the Graph2d. Maybe we can reuse and improve this? This would be awesome.

So: I have nothing to say against your changes, but think it would be better to generalize the option handling for the whole library.

@wimrijnders
Copy link
Contributor Author

Thank you for your vote of confidence. I honestly appreciate it.

The idea of making this a global change over vis.js is daunting for me. If you don't mind, I would like to concentrate on the graph3d part right now.

But I will certainly take a look at how the 2d part is doing it. Chances are, that part already has the solutions that I am considering.

@wimrijnders
Copy link
Contributor Author

OK, I had a look at the graph2d options; I see that the types of the fields are explicitly mentioned, so there must be some kind of type checking going on.

Also, I see lists for several options, which probably means that the values for those fields are restricted to the list contents. Also a pretty good idea, but I don't agree with the way it is implemented. A restricted set of values is not the same as a default value. The lists should be part of the definition of the fields, not in the list of defaults.....this assuming that I'm reading the structures correctly.

But this is way out of my current scope. Would you mind if I skip any attempt at merging the options for now? I'll make a note so I can return to it when I'm done with this sequence of PR's.

I would appreciate it if you accept this PR so I can add further changes. I can promise you the next step will look more like the graph2d options.

Copy link
Member

@yotamberk yotamberk left a comment

Choose a reason for hiding this comment

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

I think this should be accepted (waiting for @mojoaxel to see your comment and merge it), but I do think further work needs to be done with the options, as you mentioned.
Lets go ahead with the PR sequence and get to dealing with the options later.
Keep on the good work!

@wimrijnders
Copy link
Contributor Author

Wrt. option handling I'd like to take its development one step at a time. Small increments.

@mojoaxel mojoaxel merged commit 674813d into visjs:develop Oct 18, 2016
@mojoaxel
Copy link
Member

@yotamberk In the tempo the PRs come in i can't test all the changes. I hope that @wimrijnders tests his chances against the working examples ;-)

I realy like where this is heading by the way 🎉

@wimrijnders
Copy link
Contributor Author

Haha! My jesting gripe is that you guys are so fast with the reviews that I barely have time to prepare the next round of changes. You are so amazingly responsive to the incoming PR's (compliment).

Yes, I test each and every change, no matter how small, before making it a PR. I regularly cycle through the demo's to see if nothing breaks - since I know what changed, I usually restrict this to the tooltip demo, which is proof enough for me.

(I want to be able do those emoticons as well; what else besides the : ones?)

@wimrijnders wimrijnders deleted the PR9 branch October 18, 2016 15:34
@mojoaxel
Copy link
Member

(I want to be able do those emoticons as well; what else besides the : ones?)

Here: http://www.webpagefx.com/tools/emoji-cheat-sheet/ Go crazy!

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.

3 participants