-
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
Improved graphs on contributors/tags pages #2619
Conversation
Hey @jywarren I have modified the graphs on contributor/tag page, the same way I have implemented in stats page. Please see these screenshots: |
Generated by 🚫 Danger |
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.
This is Great work !! Thanks 😄 💯 !
@@ -3,37 +3,58 @@ | |||
<% if @tag %> | |||
<div id="note-graph" style="height:100px;"></div> | |||
|
|||
<script src="https://cdn.jsdelivr.net/npm/frappe-charts@0.0.8/dist/frappe-charts.min.iife.js"></script> |
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.
Hi @Souravirus !
Do you think we should put this in bower.json
file ? See this line :
Line 28 in d37c7c0
"chart.js": "v2.7.0", |
The existing chart.js library is used in this way . I think this will cause fast rendering of graphs and also will help us to easily manage newer versions .
What do you think ?
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.
Okk thanks @sagarpreet-chadha I will see to it. Thanks!!
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.
@sagarpreet-chadha Actually I tried this with bower but the frappe-chart version on bower are not that perfect and are always giving one error or other. So, I guess this way is the best we can do. I also previously tried with frappe-charts gems but that was also not properly maintained and was giving errors.
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.
Okay makes sense 👍 👍 👍 !
Hey @jywarren please review this PR |
Ooh, this looks really nice. Actually i wonder if we could make it possible to click on the graph and get a time range!!! Integrating with this: #2618 What do you think? We could open a new issue for that and merge this, if it sounds good? |
Yeah @jywarren, I really like the idea. We can implement it. So, should I add a button or something to filter out year, monthly or weekly data like you have shown in #2439 or something else.If you suggest I could also try to make the clicking on the graph feature. I guess the filter button feature was also good. |
You're right, let's do the button first, it's easier. Then we can circle back and try the click-the-graph approach. Thanks!! |
Merged!!! 💪 🙌 🎉 |
See follow-up in #2439 too! |
* Modified some files to remake the graphs of tags location * Modified some files * Modified graphs of contributors/tag * Removed the debugging lines * removed a console.log from contributors.html.erb
fixing issue #2068
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test
fixes #0000
-style reference to original issue #@publiclab/reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.
Thanks!