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

Closes #49: Add chart showing memberships over time to about page #164

Closed
wants to merge 5 commits into from

Conversation

khdc-me
Copy link
Contributor

@khdc-me khdc-me commented Jan 7, 2018

Relevant Issue & Description

Closes #49: Add chart showing memberships over time to about page

xlabels = []
is_first_record = True

membership_history = Membership.objects.values_list('member_count', 'timestamp')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to do two things here:

  1. We can actually do this work in SQL (which would be much faster and require less logic)

In SQL (which I am not sure how to convert to the django ORM yet)

SELECT timestamp::date, member_count
FROM slack_membership
  WHERE timestamp::date IN (
    SELECT date_trunc('month', timestamp)::date AS month
    FROM slack_membership GROUP BY month
  )
ORDER BY timestamp;

which with real data gives us:

 timestamp  | member_count 
------------+--------------
 2017-08-01 |         7491
 2017-09-01 |         8350
 2017-10-01 |         9107
 2017-11-01 |         9712
 2017-12-01 |        10308
 2018-01-01 |        10842
  1. Make this async (probably expose an api endpoint to get this data) - which I can do after this PR. I am planning to migrate the timezone stuff over too.

Secondly, the code should not manually convert this to JSON - that suggest the function knows how the result will be used (which really it shouldn't). Though I understand the reasoning for it

If we were going to pursue this way, I would suggest using the | tojson template tag we have in core/templatetags/tojson.py - but unfortunately that doesn't work for us since the JS is now separated from the templates more and goes through a build system (see the later comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Agreed. I wasn't sure how to do it w/ an ORM, TBH.
  2. "I am planning to migrate the timezone stuff over too" - yeah, saw you record as an issue a couple hrs ago. Retrieving directly from API would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just remembered something...
I wanted to pull dailies b/c (especially for first yr or 2), having a single number representing a whole month makes the graph jagged. With dailies, it looks a little more organic, but there's absolutely no need to place a date for each data pt.

@@ -0,0 +1,55 @@

<script src="https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.1/Chart.min.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.

We are now using yarn and webpack to build our frontend, so this should be made a dependency in the package.json with: yarn add chart.js (or whatever the library is named).

It's also kind of a bad practice to sprinkle script tags around html - generally it should either be at the beginning (and made async or deferred) or just before the body close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I tried splitting this up, following the how the map was implemented, but I think it boiled down to not understanding how to use webpack (or how to construct the json, frankly). Tried to follow what was already there but nothing would work. I'll give it another shot tomorrow.

Copy link
Contributor

@mattrasband mattrasband Jan 7, 2018

Choose a reason for hiding this comment

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

Sounds good! I am on a branch trying to make the current example a (little) better - maybe this will help:

<script id="slack_member_tz_count" type="application/json">
{{ slack_member_tz_count | tojson }}
</script>

Basically: set the JSON in a <script type="application/json" id="your_data_id">{{ thing | tojson }}</script> and retrieve it in the JS, like this:

let data = JSON.parse($('#slack_member_tz_count').html());
setupMemberMap(data);

To run the JS build see Item 7 under here

</canvas>
</div>

<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using a build system to build/minify our code we don't include scripts right in the template (instead it should be made a component in the existing client/slack/slack.js and triggered on the page being ready (currently done via jquery: $(() => { console.log('this code will not run until the page is ready!') }))

Note: please don't follow the example of how I am passing the data over, that was meant as a short-term stop gap and it will be fixed shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Will give it another go tomorrow.
Thanks for the feedback.

@khdc-me
Copy link
Contributor Author

khdc-me commented Jan 11, 2018

Thanks @joesanford , I'll make those changes sometime today.

@mattrasband
Copy link
Contributor

Closing for now after our discussion in slack about the linear-ness doesn't reflect quite what we had hoped. We sincerely appreciate the effort and time spent on this, so please don't take this as any discouragement!

@khdc-me
Copy link
Contributor Author

khdc-me commented Jan 18, 2018

Yeah, was somewhat expecting this to close after I saw that graph w/ the real data ><.

I think I found a way to represent this data effectively. I'll post some screenshots in Slack as soon as I have some time to put it together and get it running locally so you can tell me + or -.

Thanks

@khdc-me khdc-me deleted the add-memhis-chart branch January 19, 2018 03:44
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