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

Use trends legend graph everywhere #4646

Merged
merged 8 commits into from
Jun 10, 2021
Merged

Use trends legend graph everywhere #4646

merged 8 commits into from
Jun 10, 2021

Conversation

alexkim205
Copy link
Contributor

@alexkim205 alexkim205 commented Jun 8, 2021

Changes

Fixes #4484.

This replaces today's non-informative Insight tables with a table display we already have for our line graphs (less the checkbox legend key)

Changes in this CR

  • Refactored rendering Insight graphs logic
  • Generalized <TrendLegend/> into <InsightsTable/>
  • Changed to using this new table view in Trends, Sessions, and Stickiness
  • Conditionally added total count column to table.
  • More typing

P.S. - I didn't change the existing table view for Insights>Sessions>Distributed Session Lengths, because the data looks a bit different than what is required for <TrendTable/> and would need super specific data munging. For this reason, I fallback to <ActionsTable/> as shown here. Let's deprecate this later (or at least refactor. It seems overdue for some cleanup)

Before

After (for Trends, Sessions, and Stickiness tabs)

Screen Shot 2021-06-09 at 1 27 39 AM

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)

@timgl timgl temporarily deployed to posthog-pr-4646 June 8, 2021 20:29 Inactive
@timgl timgl temporarily deployed to posthog-pr-4646 June 9, 2021 07:59 Inactive
@alexkim205 alexkim205 self-assigned this Jun 9, 2021
@timgl timgl temporarily deployed to posthog-pr-4646 June 9, 2021 08:16 Inactive
@alexkim205 alexkim205 requested review from mariusandra and removed request for macobo June 9, 2021 08:29
@timgl timgl temporarily deployed to posthog-pr-4646 June 9, 2021 08:29 Inactive
@alexkim205 alexkim205 changed the title Reusing legend table for Insight Trends viz Refactor trends legend table into general use table graph Jun 9, 2021
@alexkim205 alexkim205 changed the title Refactor trends legend table into general use table graph Refactor trends legend into general use table graph Jun 9, 2021
@alexkim205 alexkim205 changed the title Refactor trends legend into general use table graph Generalize trends legend graph for multipurpose use Jun 9, 2021
@alexkim205 alexkim205 changed the title Generalize trends legend graph for multipurpose use Use trends legend graph everywhere Jun 9, 2021
@timgl timgl temporarily deployed to posthog-pr-4646 June 10, 2021 03:16 Inactive
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

🚢 great stuff! made some minor changes to follow naming conventions and decided to feature flag the total column because we need to handle the special case of unique users, because the plain sum could cause confusion (as it would likely lead to duplicate users because it's not unique across the entire period). Let's discuss this in a separate PR.

@paolodamico paolodamico merged commit a64212d into master Jun 10, 2021
@paolodamico paolodamico deleted the feat/table-insights branch June 10, 2021 03:30
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.

Table display in insights
3 participants