Skip to content

Conversation

@antgonza
Copy link
Member

There are a lot of libraries/files being updated and moved that do not need review; reviewers should focus on:

  • qiita_pet/static/js/networkVue.js
  • qiita_pet/static/vendor/README.md
  • qiita_pet/templates/sitebase.html
  • qiita_pet/templates/workflows.html

@antgonza antgonza requested a review from wasade April 28, 2021 14:09
Copy link
Contributor

@wasade wasade left a 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!!

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 );
Copy link
Contributor

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:

Suggested change
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);

Copy link
Member Author

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).

Copy link
Contributor

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 :)

var data = evt.target.data();
var element_id = data.id;

if (data.group == 'artifact') {
Copy link
Contributor

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?

if (data.group == 'artifact') {
vm.populateContentArtifact(element_id);
} else if (clickedNode.group == 'deleting') {
} else if (data.group == 'deleting') {
Copy link
Contributor

Choose a reason for hiding this comment

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

same same

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.

Thank you @wasade!

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 );
Copy link
Member Author

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).

@antgonza antgonza mentioned this pull request Apr 29, 2021
@wasade wasade merged commit 2434016 into qiita-spots:dev Apr 29, 2021
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