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

Fix bug where we can't see counts in Insight Table #4706

Merged
merged 5 commits into from
Jun 21, 2021

Conversation

alexkim205
Copy link
Contributor

Changes

Fixes #2983

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-4706 June 10, 2021 23:22 Inactive
@timgl timgl temporarily deployed to posthog-pr-4706 June 11, 2021 14:49 Inactive
@paolodamico
Copy link
Contributor

Think it's worth considering already changes from #4700

@alexkim205
Copy link
Contributor Author

alexkim205 commented Jun 11, 2021

Think it's worth considering already changes from #4700

That PR is so nice, love the UI changes. I think my fix in this PR would still apply because I'm not editing the InsightsTable component itself, rather just passing the filter into the <BindLogic/> which wraps both <InsightsTableV1/> and <InsightsTableV2/>.

I'm also going to generalize the cypress tests in my PR so that we don't have to touch them again once we move from V1 to V2.

@alexkim205 alexkim205 requested a review from paolodamico June 11, 2021 20:44
@alexkim205 alexkim205 self-assigned this Jun 11, 2021
@timgl timgl temporarily deployed to posthog-pr-4706 June 11, 2021 21:39 Inactive
@alexkim205 alexkim205 marked this pull request as ready for review June 11, 2021 21:39
<InsightsTable />
<InsightsTable
showTotalCount={
featureFlags[FEATURE_FLAGS.NEW_TOOLTIPS] &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this bugfix something we want to feature flag? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

We're not feature flagging the bug fix, showTotalCount is used to determine if we should display the new total count column in the table view, so I think it's fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, but we just show it to those who are using the new experimental UI, even though the bug is there for everyone

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

I'm probably doing something dumb here, but with this query

a) if I have no feature flags enabled, I see just a broken table
b) if I have the new tooltips flag enabled, I see this:

image

even if there is actually some data:

image

@alexkim205
Copy link
Contributor Author

@mariusandra

a. The bug exists in both the old and new ui, so hiding this behind a feature flag doesn't make sense. I'll remove that check.
b. I clicked the link with the feature flag enabled but can't reproduce the issue. I tried to use some data that had similar shape (where data only exists on last day) here but it looks like all the data's there 🤔

@alexkim205 alexkim205 requested a review from mariusandra June 21, 2021 16:00
@mariusandra mariusandra merged commit 8e30172 into master Jun 21, 2021
@mariusandra mariusandra deleted the fix/trends-table-no-data branch June 21, 2021 21:21
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.

Show Cohort name instead of number on table views
4 participants