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.

Sign up for GitHub

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

Jump to bottom

update vis-network #2969

Merged
merged 8 commits into from
Jan 22, 2020
Merged

update vis-network #2969

merged 8 commits into from
Jan 22, 2020

Conversation

antgonza
Copy link
Member

No description provided.

@codecov-io
Copy link

codecov-io commented Jan 12, 2020 •
edited
Loading

Codecov Report

Merging #2969 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #2969   +/-   ##
=======================================
  Coverage   95.02%   95.02%           
=======================================
  Files          74       74           
  Lines       14088    14088           
=======================================
  Hits        13387    13387           
  Misses        701      701

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7229d26...0cde4a4. Read the comment docs.

@antgonza antgonza mentioned this pull request Jan 13, 2020
Closed
@antgonza
Copy link
Member Author

@justinshaffer, I deployed this version in qiita-rc and it will be great if you could tests that everything seems fine (running the tutorials should suffice). Thank you!

ElDeveloper
ElDeveloper approved these changes Jan 13, 2020
View reviewed changes
Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

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

Looks good. Do you have an intuition for changing the values in the network initialization?

@antgonza
Copy link
Member Author

@ElDeveloper, thank you for the review. Note that during @justinshaffer review in qiita-rc, he found that there is some kind of bug while using the new version and we are evaluating if we want to do try to have this PR for this release or next.

Now, answering your question:

  • sortMethod: "directed": completely changes behavior in the new version and is better to use the default setting.
  • node-size/levelSeparation: it's just to save space (shrink separation between nodes)

@ElDeveloper
Copy link
Member

ElDeveloper commented Jan 13, 2020 via email

@antgonza
Copy link
Member Author

@ElDeveloper, this version is deployed in qiita-rc and @justinshaffer and I have tested and seems to be fine; could you do another review? Thank you!

ElDeveloper
ElDeveloper suggested changes Jan 21, 2020
View reviewed changes
Copy link
Member

@ElDeveloper ElDeveloper left a comment

Choose a reason for hiding this comment

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

Looks good, I've made a few comments. The majority of them are fairly straightforward.

qiita_pet/templates/sitebase.html Outdated
@@ -20,6 +20,8 @@
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en-US">
<head>
<title>{{portal_styling.title}}</title>
<!-- https://unpkg.com/browse/[email protected]/styles/vis-network.min.css -->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- https://unpkg.com/browse/[email protected]/styles/vis-network.min.css -->

Copy link
Member Author

Choose a reason for hiding this comment

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

I left there so we know where that file came from, should I added somewhere else; like the README?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest putting these links in the License file for this dependency. I think there should be a markdown file in the vendor directory.

qiita_pet/templates/sitebase.html Outdated
@@ -45,7 +45,8 @@
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/jquery.dataTables.plugin.natural.js" type="text/javascript"></script>
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/vue.min.js"></script>
<script src="{% raw qiita_config.portal_dir %}/static/js/qiita.js"></script>
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/vis.min.js"></script>
<!-- https://unpkg.com/browse/[email protected]/standalone/umd/vis-network.min.js -->
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!-- https://unpkg.com/browse/[email protected]/standalone/umd/vis-network.min.js -->

Copy link
Member Author

Choose a reason for hiding this comment

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

Same that before; know where it comes from.

qiita_pet/static/js/networkVue.js
@@ -713,22 +713,12 @@ Vue.component('processing-graph', {
**/
addJobNodeToGraph: function(job_info) {
let vm = this;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this line is no longer used.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do as we need to add vm.new_job_info below.

qiita_pet/static/js/networkVue.js Outdated
@@ -974,11 +977,16 @@ Vue.component('processing-graph', {
// data in a way that Vis.Network likes it
// Format edge list data
for(var i = 0; i < data.edges.length; i++) {
// forcing a string
data.edges[i][0] =data.edges[i][0].toString()
data.edges[i][1] =data.edges[i][1].toString()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data.edges[i][1] =data.edges[i][1].toString()
data.edges[i][1] = data.edges[i][1].toString()

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks.

antgonza
antgonza commented Jan 21, 2020
View reviewed changes
Copy link
Member Author

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

@ElDeveloper, thank you for the review; I have added some question on how to proceed ...

qiita_pet/static/js/networkVue.js
@@ -713,22 +713,12 @@ Vue.component('processing-graph', {
**/
addJobNodeToGraph: function(job_info) {
let vm = this;
Copy link
Member Author

Choose a reason for hiding this comment

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

We do as we need to add vm.new_job_info below.

qiita_pet/static/js/networkVue.js Outdated
@@ -974,11 +977,16 @@ Vue.component('processing-graph', {
// data in a way that Vis.Network likes it
// Format edge list data
for(var i = 0; i < data.edges.length; i++) {
// forcing a string
data.edges[i][0] =data.edges[i][0].toString()
data.edges[i][1] =data.edges[i][1].toString()
Copy link
Member Author

Choose a reason for hiding this comment

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

thanks.

qiita_pet/templates/sitebase.html Outdated
@@ -20,6 +20,8 @@
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en-US">
<head>
<title>{{portal_styling.title}}</title>
<!-- https://unpkg.com/browse/[email protected]/styles/vis-network.min.css -->
Copy link
Member Author

Choose a reason for hiding this comment

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

I left there so we know where that file came from, should I added somewhere else; like the README?

qiita_pet/templates/sitebase.html Outdated
@@ -45,7 +45,8 @@
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/jquery.dataTables.plugin.natural.js" type="text/javascript"></script>
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/vue.min.js"></script>
<script src="{% raw qiita_config.portal_dir %}/static/js/qiita.js"></script>
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/vis.min.js"></script>
<!-- https://unpkg.com/browse/[email protected]/standalone/umd/vis-network.min.js -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Same that before; know where it comes from.

@ElDeveloper ElDeveloper merged commit c4da371 into qiita-spots:dev Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElDeveloper ElDeveloper ElDeveloper approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antgonza @codecov-io @ElDeveloper