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

Add initial charts implementation to pro dashboard #2037

Merged

Conversation

Zhao-Andy
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Feature

Description

This PR adds some basic time-series charts to the pro dashboard. It also lays some groundwork for code to be reused for other charts/data-visualizations.

Code/implementation-detail-wise there are a lot of individual queries, which would grow as we want more and different kinds of data. What we might want to do is move them into javascript_pack_tags that call an API endpoint for the specific data, and then have it render afterward.

There's more to add for sure, but I think this will be a good start.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

charts dashboard example

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Mar 11, 2019
class ChartDecorator < Draper::CollectionDecorator
def total_by_type_per_day(options)
case options[:type]
when "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.

This when statement is not being used, but will probably be used in the next implementation. I wrote it more as an example.

@shindakun
Copy link
Contributor

Oh nice! What qualifies one as a pro? And let me know if you need beta testers. ;)

Co-Authored-By: Zhao-Andy <andyzhao.zhao@gmail.com>
app/views/dashboards/pro.html.erb Outdated Show resolved Hide resolved
app/views/dashboards/pro.html.erb Outdated Show resolved Hide resolved
app/views/dashboards/pro.html.erb Outdated Show resolved Hide resolved
app/views/dashboards/pro.html.erb Outdated Show resolved Hide resolved
app/views/dashboards/pro.html.erb Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Mar 11, 2019
Co-Authored-By: Zhao-Andy <andyzhao.zhao@gmail.com>
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Mar 11, 2019
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Mar 11, 2019
@last_week_reactions_count = Reaction.where(reactable_id: current_user.articles.pluck(:id), reactable_type: "Article").where("created_at > ? AND created_at < ?", 2.weeks.ago, 1.week.ago).size
@this_month_reactions_count = Reaction.where(reactable_id: current_user.articles.pluck(:id), reactable_type: "Article").where("created_at > ?", 1.month.ago).size
@last_month_reactions_count = Reaction.where(reactable_id: current_user.articles.pluck(:id), reactable_type: "Article").where("created_at > ? AND created_at < ?", 2.months.ago, 1.months.ago).size
@current_user_article_ids = current_user.articles.pluck(:id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using a facade pattern here instead of using lots of instance variables.
Like creating a class Dashboards::Pro (or another name) with the corresponding methods.
Example

Copy link
Contributor

Choose a reason for hiding this comment

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

  • that would allow to add a separate test for the data easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense. @Zhao-Andy refactoring as such sooner rather than later is probably good.

@benhalpern
Copy link
Contributor

@shindakun right now this is pretty secluded because we haven't built this to scale to very many users. The question of what qualifies as "pro" is to be determined, and a lot of these components will be general availability depending on what scales easily and what doesn't.

I'll make sure you get on the list for early access.

@benhalpern benhalpern merged commit 1922a55 into forem:master Mar 12, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes PR: merged bot applied label for PR's that are merged labels Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: reviewed-approved bot applied label for PR's where reviewer approves changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants