Add min/max zoom support

[gregdeane]

Closes #574

[Thomaash]

NaN shouldn't be allowed and the configurator configuration is wrong. Otherwise it looks good.

[gregdeane]

@Thomaash Made the requested changes. I hope I got the ranges right.

[Thomaash]

It still doesn't refuse NaN. Even though NaN works as no limit, it's a very bad idea to have NaN just floating around in unexpected places. This shouldn't be allowed to pass through setOptions.

[Thomaash]

I don't get this. Why is the range so small?.

[gregdeane]

Mainly because min/max are separate props. What I mean is: Since there is a min prop, I thought it would be strange to put a very low number in the max prop.

I'm happy to put the range you think is appropriate. What's your suggestion for both props?

[Thomaash]

I'd go with 0 through 10 for both. The input validation should handle cases where max is smaller than min etc.

[gregdeane]

Will do, thank you.

[gregdeane]

It still doesn't refuse NaN. Even though NaN works as no limit, it's a very bad idea to have NaN just floating around in unexpected places. This shouldn't be allowed to pass through setOptions.

Ah, sorry I missed that one.

[gregdeane]

It still doesn't refuse NaN. Even though NaN works as no limit, it's a very bad idea to have NaN just floating around in unexpected places. This shouldn't be allowed to pass through setOptions.

@Thomaash I'm having a little trouble understanding where exactly to handle the NaN. I see that the Validator catches the fact that a string should not be used (for example).

In InteractionHandler.setOptions(), I could check if the values exist and if they are anything but numbers, use default values instead. But this seems a bit hacky.

Can you explain a bit more what you want to see and point me to the code where you think it should go?

[Thomaash]

It still doesn't refuse NaN. Even though NaN works as no limit, it's a very bad idea to have NaN just floating around in unexpected places. This shouldn't be allowed to pass through setOptions.

@Thomaash I'm having a little trouble understanding where exactly to handle the NaN. I see that the Validator catches the fact that a string should not be used (for example).

In InteractionHandler.setOptions(), I could check if the values exist and if they are anything but numbers, use default values instead. But this seems a bit hacky.

Can you explain a bit more what you want to see and point me to the code where you think it should go?

It isn't a bit hacky, it's very hacky.

Yeah, basically check if the value is fine and if not, just ignore it (as if it wasn't even there in the first place, don't reset it to default value simply because there's invalid input) and console error a message about it.

[gregdeane]

Yeah, basically check if the value is fine and if not, just ignore it (as if it wasn't even there in the first place, don't reset it to default value simply because there's invalid input) and console error a message about it.

In the InteractionHandler.setOptions() method? Where exactly is the "non-hacky" place to do this?

[Thomaash]

Yeah, basically check if the value is fine and if not, just ignore it (as if it wasn't even there in the first place, don't reset it to default value simply because there's invalid input) and console error a message about it.

In the InteractionHandler.setOptions() method? Where exactly is the "non-hacky" place to do this?

There's no non hacky place. There is some replacement in the workings, but that's stalled due to lack of time. Just trial-and-error a place in the method where it works and don't worry about it.

[gregdeane]

@Thomaash Please have another look. Hope this solution fits the bill.

[Thomaash]

I have no idea what you're trying to achieve here. typeof zoomMin !== undefined is always true, typeof never returns undefined. Also typeof NaN === "number". And finally you validate the old options and then merge the new unvalidated options into the old ones? You have to validate options before you merge it into this.options.

[gregdeane]

@Thomaash Apologies for any misunderstandings. This is what I plan on doing. What do you think?

setOptions(options) {
  if (options !== undefined) {
    // remove zoomMin and/or zoomMax if they aren't numbers
    let { zoomMin, zoomMax } = options;

    if (isNaN(zoomMin)) {
      delete options.zoomMin;
    }

    if (isNaN(zoomMax)) {
      delete options.zoomMax;
    }

    // extend all but the values in fields
    let fields = ['hideEdgesOnDrag', 'hideEdgesOnZoom', 'hideNodesOnDrag','keyboard','multiselect','selectable','selectConnectedEdges'];
    util.selectiveNotDeepExtend(fields, this.options, options);

    // merge the keyboard options in.
    util.mergeOptions(this.options, options, 'keyboard');

    if (options.tooltip) {
      util.extend(this.options.tooltip, options.tooltip);
      if (options.tooltip.color) {
        this.options.tooltip.color = util.parseColor(options.tooltip.color);
      }
    }
 }

  this.navigationHandler.setOptions(this.options);
}

Additionally, based on this description, do you think we need to use Number.isNaN() or the following polyfill?

var isNaN = function(value) {
    var n = Number(value);
    return n !== n;
};

If you aren't satisfied with the above plan, please tell me exactly what you want.

Thanks!

[Thomaash]

@Thomaash Apologies for any misunderstandings. This is what I plan on doing. What do you think?

setOptions(options) {
  if (options !== undefined) {
    // remove zoomMin and/or zoomMax if they aren't numbers
    let { zoomMin, zoomMax } = options;

    if (isNaN(zoomMin)) {
      delete options.zoomMin;
    }

    if (isNaN(zoomMax)) {
      delete options.zoomMax;
    }

    // extend all but the values in fields
    let fields = ['hideEdgesOnDrag', 'hideEdgesOnZoom', 'hideNodesOnDrag','keyboard','multiselect','selectable','selectConnectedEdges'];
    util.selectiveNotDeepExtend(fields, this.options, options);

    // merge the keyboard options in.
    util.mergeOptions(this.options, options, 'keyboard');

    if (options.tooltip) {
      util.extend(this.options.tooltip, options.tooltip);
      if (options.tooltip.color) {
        this.options.tooltip.color = util.parseColor(options.tooltip.color);
      }
    }
 }

  this.navigationHandler.setOptions(this.options);
}

Additionally, based on this description, do you think we need to use Number.isNaN() or the following polyfill?

var isNaN = function(value) {
    var n = Number(value);
    return n !== n;
};

If you aren't satisfied with the above plan, please tell me exactly what you want.

Thanks!

Polyfilling is done automatically, you can just use Number.isNaN(), isNaN() is just weird.

It's better to test for valid values (you can always negate it with !(…)) rather than invalid ones. The way you do it is very prone to forgetting some invalid values. For example what does -7 mean for zoom? It's valid according to your code.

Also use const if you don't reassign it later on.

[gregdeane]

@Thomaash Made the updates.

[gregdeane]

Just checking in on this. Any way we can merge?

[gregdeane]

@Thomaash Can we merge this?

[yotamberk]

@Thomaash Can you re-review this?
it seems like the changes have been done

[Thomaash]

Sorry for the delay, can't find any time for this project lately. It looks good except for one (hopefully) last thing: If I set the zoom limits it's not enforced right away. The user has to zoom for the limit to take action. For example if min zoom is 5 the network shouldn't open with zoom 1.

[gregdeane]

Sorry for the delay, can't find any time for this project lately. It looks good except for one (hopefully) last thing: If I set the zoom limits it's not enforced right away. The user has to zoom for the limit to take action. For example if min zoom is 5 the network shouldn't open with zoom 1.

@Thomaash I've having a bit of trouble finding the spot where this change should be made. I've narrowed it down to here but I'm not convinced this is the correct place. At this point, options contains the interaction prop (if set).

My thought was that if I am able to merge zoomMin into the options object at this point, then I can use it to set scale here.

It would be of great help if you could point me in the right direction and perhaps even suggest how I might implement the change.

Thanks!

[Thomaash]

Ideally all changes to scale would be handled in one place (View.js seems like a good candidate) and receive requests from other places (e.g. this.body.emitter.emit("scale", { to: 4.7 }) or this.body.emitter.emit("scale", { by: -0.3, at: { x: 77, y: 234 } })). Then there wouldn't be the problem that the same configuration and logic has to be in multiple places.

I don't know how eager you are to dive into this. However I don't see much of an alternative here. You could keep it fragmented as it is now but then you would still need to establish some form of communication because there's also fit, resizing and maybe some other scale altering stuff I can't remember right now.

[gregdeane]

Ideally all changes to scale would be handled in one place (View.js seems like a good candidate) and receive requests from other places (e.g. this.body.emitter.emit("scale", { to: 4.7 }) or this.body.emitter.emit("scale", { by: -0.3, at: { x: 77, y: 234 } })). Then there wouldn't be the problem that the same configuration and logic has to be in multiple places.

I don't know how eager you are to dive into this. However I don't see much of an alternative here. You could keep it fragmented as it is now but then you would still need to establish some form of communication because there's also fit, resizing and maybe some other scale altering stuff I can't remember right now.

Just want to let you know that I haven't forgotten about this PR. Work has been insanely busy lately and will likely continue to be for another week or two. I'll jump back on this PR as soon as I can.

[gregdeane]

@Thomaash Unfortunately, my schedule won't be cleared up for quite some time. If there's a break, I'll get back on this feature. Otherwise, I'm out for the foreseeable future. This will most likely be the case.

Please understand that the following is detailed not with an accusatory tone but with a friendly tone. No need to take offense.

I suggest that for future PR's, everything you wish to see is commented upon during the first review. You want things a very specific way and that's fine. But it would have been beneficial to let me know in the very beginning exactly what you wanted. The back-and-forth went on much too long in my opinion and made it more difficult to continue working on this feature.

You may lay blame on me for not having the code properly written or up to vis-network standards. Again, fine. Just explain what you expect during the first review. If you wish to have something written a specific way, say as much. Just provide an example (e.g. code snippet) of what you want.

It should not take so long to contribute a small change, in my opinion.

Anyway, I hope to find time to address this topic again. But if not, I hope someone else will finish it up.

[Thomaash]

Thanks for the feedback. It makes a lot of sense and I'll try to give better and more detailed reviews right away. Anyway it's fine if you can't find the time to work on this, I can understand that.

[gregdeane]

@Thomaash And thank you for your understanding and consideration on the feedback. Much appreciated!

[Jacse]

What's the status of this? @Thomaash are you taking over or do you need someone else to? I'm guessing you'll want consistency with #1134 which you just merged.

[inmylo]

Any updates? @gregdeane , @Thomaash

[gregdeane]

@inmylo Unfortunately, the time involved in trying to get this PR approved went beyond what I was able to allocate. Additionally, I believe the base code is (somewhat) different now and even if this PR was approved, it wouldn't be possible to merge.

Best bet is to have someone else take it over, adapt it to the newer code, and open a new PR.

I've now closed this PR.