-
Notifications
You must be signed in to change notification settings - Fork 79
New network #3096
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
New network #3096
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor stuff, otherwise 👍, thanks!!
qiita_pet/static/js/networkVue.js
Outdated
| vm.network.on("stabilized", function (params) { | ||
| // Note: we only need to sort the edges to keep the same structure of the | ||
| // graph; in other words, nodes order is not important | ||
| vm.edges = vm.edges.sort((a, b) => (a.data.source > b.data.source) ? 1 : (a.data.source === b.data.source) ? ((a.data.target > b.data.target) ? 1 : -1) : -1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triple nested ternary is a bit hard... any chance this could be refactored? I think it would be:
| vm.edges = vm.edges.sort((a, b) => (a.data.source > b.data.source) ? 1 : (a.data.source === b.data.source) ? ((a.data.target > b.data.target) ? 1 : -1) : -1 ); | |
| var sort_order = -1; | |
| if ((a.data.source > b.data.source) || ((a.data.source === b.data.source) && (a.data.target > b.data.target)) { | |
| sort_order = 1; | |
| } | |
| vm.edges = vm.edges.sort(sort_order); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, just note that this is a function like in python list.sort(key=sorting_function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, so pack into a function... but main item was, if okay, to expand the nested ternaries :)
qiita_pet/static/js/networkVue.js
Outdated
| var data = evt.target.data(); | ||
| var element_id = data.id; | ||
|
|
||
| if (data.group == 'artifact') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor i think here, but possible to use === to be consistent with the other equalities?
qiita_pet/static/js/networkVue.js
Outdated
| if (data.group == 'artifact') { | ||
| vm.populateContentArtifact(element_id); | ||
| } else if (clickedNode.group == 'deleting') { | ||
| } else if (data.group == 'deleting') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @wasade!
qiita_pet/static/js/networkVue.js
Outdated
| vm.network.on("stabilized", function (params) { | ||
| // Note: we only need to sort the edges to keep the same structure of the | ||
| // graph; in other words, nodes order is not important | ||
| vm.edges = vm.edges.sort((a, b) => (a.data.source > b.data.source) ? 1 : (a.data.source === b.data.source) ? ((a.data.target > b.data.target) ? 1 : -1) : -1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, just note that this is a function like in python list.sort(key=sorting_function).
There are a lot of libraries/files being updated and moved that do not need review; reviewers should focus on: