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.

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

Merged
merged 5 commits into from
Apr 23, 2018

Conversation

Souravirus
Copy link
Member

@Souravirus Souravirus commented Apr 15, 2018

fixing issue #2068
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If 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!

@Souravirus
Copy link
Member Author

Hey @jywarren I have modified the graphs on contributor/tag page, the same way I have implemented in stats page. Please see these screenshots:
screenshot from 2018-04-15 22-48-37
screenshot from 2018-04-15 22-48-57

@PublicLabBot
Copy link

PublicLabBot commented Apr 15, 2018

1 Message
📖 @Souravirus Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.

Generated by 🚫 Danger

Copy link
Contributor

@sagarpreet-chadha sagarpreet-chadha left a 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>
Copy link
Contributor

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 :

"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 ?

Copy link
Member Author

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!!

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay makes sense 👍 👍 👍 !

@Souravirus
Copy link
Member Author

Hey @jywarren please review this PR

@jywarren
Copy link
Member

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?

@Souravirus
Copy link
Member Author

Souravirus commented Apr 20, 2018

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.

@jywarren
Copy link
Member

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!!

@jywarren jywarren merged commit 499bb8d into publiclab:master Apr 23, 2018
@jywarren jywarren removed the ready label Apr 23, 2018
@jywarren
Copy link
Member

Merged!!! 💪 🙌 🎉

@jywarren
Copy link
Member

See follow-up in #2439 too!

@Souravirus Souravirus deleted the contributorsTag branch June 28, 2018 19:48
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* 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
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.

None yet

5 participants