-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[WIP] Continued refinements to cytoscape visualization #9902
Conversation
app/models/tag.rb
Outdated
} | ||
end | ||
data["edges"] = [] | ||
data["tags"].each do |tag| | ||
Tag.related(tag["name"], 10).each do |related_tag| | ||
data["edges"] << { "from" => tag["name"], "to" => related_tag.name } | ||
reverse = {"from" => related_tag.name, "to" => tag["name"]} |
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.
Space inside { missing.
app/models/tag.rb
Outdated
} | ||
end | ||
data["edges"] = [] | ||
data["tags"].each do |tag| | ||
Tag.related(tag["name"], 10).each do |related_tag| | ||
data["edges"] << { "from" => tag["name"], "to" => related_tag.name } | ||
reverse = {"from" => related_tag.name, "to" => tag["name"]} |
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.
Space inside } missing.
app/models/tag.rb
Outdated
} | ||
end | ||
data["edges"] = [] | ||
data["tags"].each do |tag| | ||
Tag.related(tag["name"], 10).each do |related_tag| | ||
data["edges"] << { "from" => tag["name"], "to" => related_tag.name } | ||
reverse = {"from" => related_tag.name, "to" => tag["name"]} | ||
unless (data["edges"].include? reverse) |
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.
Don't use parentheses around the condition of an unless
.
app/models/tag.rb
Outdated
@@ -387,13 +387,16 @@ def self.graph_data(limit = 250) | |||
.limit(limit).each do |tag| | |||
data["tags"] << { | |||
"name" => tag.name, | |||
"count" => tag.count | |||
"count" => tag.count, |
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.
Avoid comma after the last item of a hash.
Codecov Report
@@ Coverage Diff @@
## main #9902 +/- ##
==========================================
+ Coverage 82.06% 82.12% +0.05%
==========================================
Files 98 98
Lines 5945 5952 +7
==========================================
+ Hits 4879 4888 +9
+ Misses 1066 1064 -2
|
4780b1b
to
ccf4c85
Compare
Code Climate has analyzed commit ccf4c85 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Just a small tweak we can do is to hide the edges when highlighting a single node on hover for some clear representation. e.g- |
@17sushmita This definitely improves the visuals a LOT! What do you think of the codeclimate issues? They seem like pretty easy fixes that aren't too complicated. |
Yes I've fixed the style issues. But the complexity issue is about maximum allowed no of lines in a method is 25 and this method is of 26 lines. How to fix this🤔. |
Got it!
Oh, I see. I was referring to these, but I now see that they're outdated so never mind: Regarding the "greater than 25" lines, I would just skip those suggestions. I guess you could get around them by collapsing queries like these into one line, but that makes readability a lot worse:
|
Love this!!!! I'll bypass the final recommendation. Is this ready for merge then? Thank you!!! |
I am planning to make more refinements to the cytoscape view. However, you can merge this one too and I will create a new PR or I'll add commits to this one only. |
and also what do you think about the above suggested change to hide the other edges on highlighting a node? Should we do this or it looks fine this way only? |
Love it. Let's do it in increments. 🎉
Yes, that looked nice too! Let's keep a bulleted list of the improvements you've made. If people like them or not, we'll have a place we can point to each distinct change that was made, which will make it easier to discuss. Thanks, @17sushmita and thanks @noi5e and all who reviewed!!! |
* test cytoscape changes on unstable * minor refactoring of previous commit
* test cytoscape changes on unstable * minor refactoring of previous commit
Fixes #9880
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test
@publiclab/reviewers
for help, in a comment belowChanges made till now-
Responsive
Before-(https://publiclab.org/stats/graph)
![image](https://user-images.githubusercontent.com/42088159/125268456-5a1bd280-e325-11eb-9498-74464bd9a9fe.png)
After-(https://unstable.publiclab.org/stats/graph)
![image](https://user-images.githubusercontent.com/42088159/125285409-3dd56100-e338-11eb-984f-96fecc462ace.png)
Mobile view before-
Mobile view after-
On hover-
![image](https://user-images.githubusercontent.com/42088159/125286577-95c09780-e339-11eb-8b09-e9e0ea639af6.png)