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

New Insights query UI, Option B #4233

Merged
merged 16 commits into from
May 7, 2021
Merged

Conversation

samwinslow
Copy link
Contributor

Changes

Adds to #4050. New query UI experiment in Insights. Note that in order to test, you must set feature flag 4050-query-ui-optB to true.

Screen Shot 2021-05-06 at 10 02 52 AM

To do

  • add Cohorts to SelectBox instance

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

@timgl timgl temporarily deployed to posthog-pr-4233 May 6, 2021 14:08 Inactive
@timgl timgl temporarily deployed to posthog-pr-4233 May 6, 2021 17:01 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.

Love the new dropdown, it just feels fast.

UX-wise found a couple of issues.

  1. Clicking the dropdown again re-opens it instead of closing it.
    image

  2. Weird behaviour with same-named user/event properties. Different for keyboard & mouse navigation.
    image

  3. Improve description for PostHog properties.

@paolodamico paolodamico marked this pull request as ready for review May 6, 2021 17:04
@timgl timgl temporarily deployed to posthog-pr-4233 May 6, 2021 18:29 Inactive
Copy link
Contributor Author

@samwinslow samwinslow left a comment

Choose a reason for hiding this comment

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

EDIT: resolved

This part also makes zero sense to me — where is id defined?

Screen Shot 2021-05-06 at 2 29 05 PM

@timgl timgl temporarily deployed to posthog-pr-4233 May 6, 2021 19:17 Inactive
@timgl timgl temporarily deployed to posthog-pr-4233 May 6, 2021 19:40 Inactive
@timgl timgl temporarily deployed to posthog-pr-4233 May 6, 2021 20:22 Inactive
@paolodamico
Copy link
Contributor

paolodamico commented May 6, 2021

Ping me when this is ready for another look 😉

I see you caught the TS error, it was actually in line 196, not sure why GH actions reported it in line 1. You can always run ./bin/check-typescript-strict locally to get more useful output.

@samwinslow
Copy link
Contributor Author

Working on Cohorts in SelectBox. I think I partially fixed the error with duplicated keys in the dropdown, although now I'm also seeing strange behavior when pressing the Down / Up keys. In the video below, I'm continually hitting Down, then Up, but as you can see the N and N-1 elements are in the wrong order. This only happens with some searches — wonder if it's related to Fuse?

Screen.Cast.2021-05-06.at.5.53.53.PM.mp4

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.

lgtm! let's get this in and continue from there.

TODOs for your next PR:

  1. Selecting a cohort doesn't actually filter out the graph.
  2. PropertyKeyInfo popover still showing for element outside the dropdown.

  1. (Unrelated to this PR). Weird behavior when alternating mouse & keyboard selection.
    image

@paolodamico paolodamico merged commit 2c78919 into master May 7, 2021
@paolodamico paolodamico deleted the 4050-query-ui-optB-propertyfilters branch May 7, 2021 00:01
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.

3 participants