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

adding missing "undefined" default values #1732

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nickchomey
Copy link

Many parameters were missing the "undefined" default value, which throws an error when such a value is passed through

image

These errors go away when these defaults are added in

many parameters were missing the "undefined" default value, which throws an error when such a value is passed through
@Thomaash
Copy link
Member

Hi @nickchomey,

what exactly are you trying to do here? How did you get these errors? What you changed are not default values but type checks. I'm not sure whether all of these things can be safely set to undefined. Your code doesn't actually work at all, did you even test it?

@nickchomey
Copy link
Author

nickchomey commented Mar 28, 2022

Sorry that you seem offended by my effort - I'm brand new to all of this and just trying to help.

As the screenshot shows, I got those errors in the browser console just by loading the network. They are generated when the network options contains the edited settings that are set to the default values shown in the documentation on the visjs site. There are many listed there with "undefined" as the default value, and one with false.

If you look at the file that I have edited, it already has type checking for undefined for many settings - e.g. nodes.borderWidthSelected. As such, I simply added this same type checking to the other settings that were missing such a type check for their documented default value.

And, yes, I did test it all (though only by directly modifying my visjs-network.min.js file) and it worked for me - as it should, given that I didn't do anything that wasn't already done. However, I do now see the automated checks below - I have made a few corrections for typos/oversights and there only seems to be a couple outstanding errors - linting and an example check. I'm not sure what to do about those.

Please let me know if you have any other questions, or suggestions on how I can better contribute.

@Thomaash
Copy link
Member

Sorry that you seem offended by my effort - I'm brand new to all of this and just trying to help.

I'm not offended, I just didn't understand what this was supposed to do exactly.

As the screenshot shows, I got those errors in the browser console just by loading the network. They are generated when the network options contains the edited settings that are set to the default values shown in the documentation on the visjs site. There are many listed there with "undefined" as the default value, and one with false.

Okay, would be great to include the actuall confing (ideally a proper MWE) right away.

If you look at the file that I have edited, it already has type checking for undefined for many settings - e.g. nodes.borderWidthSelected. As such, I simply added this same type checking to the other settings that were missing such a type check for their documented default value.

I see, in general it should be possible to set the default value explicitly.

And, yes, I did test it all (though only by directly modifying my visjs-network.min.js file) and it worked for me - as it should, given that I didn't do anything that wasn't already done. However, I do now see the automated checks below - I have made a few corrections for typos/oversights and there only seems to be a couple outstanding errors - linting and an example check. I'm not sure what to do about those.

Directly modifying minified files is a pretty crazy idea, why would you do something like that? Given that, you didn't test it then, you tested something else than what you committed.

One of the failing tests should be as easy to fix as npm run style-fix && npm run lint-fix. The other one is due to one of the examples not rendering anything (i.e. you broke something with this change).

@nickchomey
Copy link
Author

nickchomey commented Mar 28, 2022

The answer to your continued incredulity is what I stated already - I'm just a new user of the library and I don't know what I dont know. I also don't have any idea how to do the entire build and testing process and I can't find any documentation on how to do so. As such, some patience would be appreciated. Again, I'm simply trying my best to help.

I don't know what an MWE is. In my script, I literally included all default settings of the config options listed in the documentation - just to have it there as easy reference/modification while experimenting.

It seems to me that if you simply create a network with options that includes any of the settings that I've added a type check for, you should receive the same errors in the browser console.

Here is an excerpt from my code (takes data from php/wordpress and displays it with visjs)

<script>
var nodes = new vis.DataSet(<?php echo wp_json_encode( $nodes ); ?>);
var edges = new vis.DataSet(<?php echo wp_json_encode( $edges ); ?>);
var container = document.getElementById('pn');
var data = {
    nodes: nodes,
    edges: edges
};
var options = {
    nodes: {
        font: {
            background: undefined,
        }
        heightConstraint: {
            minimum: undefined 
        },
    }
}
var pn = new vis.Network(container, data, options);
</script>

It seems to me this PR is 95+% of the way complete. And, again, this shouldn't be modifying any functionality in any way - I've only added type checks that already exist in the code and presumably should already be there based on what the documentation says. So, to the extent that it breaks one example, perhaps it means that the documentation is incorrect with respect to the default values or there is another bug that this is uncovering. But I don't think I'm capable of going any further with this, so if you would prefer me to just close this and open it as an issue, I can do that as well.

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.

2 participants