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 filtering experience UI enhancements #4296

Merged
merged 16 commits into from
May 12, 2021
Merged

Conversation

samwinslow
Copy link
Contributor

@samwinslow samwinslow commented May 11, 2021

Changes

Contributes to #4050:

  • Filtering experience. Local filters should be collapsed by default (opened through filter icon at the right of each row). Add more filters by clicking a plus button (see wireframes).
  • UI of filter rows, zebra-striped to clearly distinguish each row (as shown on mockup).
  • Change filter icon to "X" to avoid confusion with removing entire series.

Screen Shot 2021-05-11 at 6 42 57 AM

  • Missing icons on display config (chart visualization type, and interval)

Screen Shot 2021-05-11 at 6 43 29 AM

Screen Shot 2021-05-11 at 6 43 37 AM

  • Remove event popover for PostHog-based events (as we did for properties).
  • Funnels. We'll probably want to keep vertical layout for them (due to the nature of the steps process, plus there's no benefit of how these are read horizontally
  • (nice to have). Tabbed keyboard navigation.

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-4296 May 11, 2021 10:49 Inactive
@samwinslow
Copy link
Contributor Author

Tests failing inconsistently / appear unrelated. Different tests fail each time.

@samwinslow samwinslow requested a review from paolodamico May 11, 2021 12:17
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.

Loving this! The new filtering button/experience is amazing and feels so fast! Also, great stuff on the display icons, wasn't picturing them that way, love them!

Some minor things, but nothing blocking (numbered for easy discussion):

  1. The zebra-striped is separating each filter but I think to be more useful it should be for each graph series group (series definition + filters) that's separated. That way if you have multiple filters on each series and multiple series, you can quickly identify which one is which.
  2. Arrow is not centered vertically.

3. Genuine question. Should we use the same tag indicator we use elsewhere for the "AND"s? Not sure if actually just plain text would be clearer here.

4. For the interval filters, I would just add the clock icon to the main label display (instead of on each option). Like we have for the date range selector. My thinking is that the icons don't really provide extra value. 5. (nice to have). With the introduction of tabbed navigation, would be pretty useful to be able to close the events and properties selectors with the "Esc" key.

@timgl timgl temporarily deployed to posthog-pr-4296 May 11, 2021 17:09 Inactive
@timgl timgl temporarily deployed to posthog-pr-4296 May 11, 2021 17:34 Inactive
@samwinslow
Copy link
Contributor Author

samwinslow commented May 11, 2021

@paolodamico for some reason I can't reply inline to your review — could you clarify this?

Actually it looks like the display selector is no longer visible on funnels. (frontend/src/lib/components/ChartFilter/ChartFilter.js)

Do you mean it's okay that the selector is no longer visible on funnels? In such a case, should we then remove this block on line 53 and just keep the second part of the ternary?

const options =
        filters.insight === ViewType.FUNNELS
            ? [
// ...
  1. Arrow is not centered vertically.

Also can't repro this — how does it look for you here on Insights when the feature flag is turned off? I suspect the problem might be because the arrow is actually a Unicode character; perhaps different browsers or OSX versions use a different system symbols font.

Screen Shot 2021-05-11 at 1 51 04 PM

  1. Yes, I like the AND tags and would be happy to use them! Related to point 2, should we keep this arrow thing at all? Perhaps an indent before "where" is all we need.

EDIT: New approach re: point 3:

Screen Shot 2021-05-11 at 3 44 34 PM

@timgl timgl temporarily deployed to posthog-pr-4296 May 11, 2021 20:10 Inactive
@paolodamico
Copy link
Contributor

Np, thanks for the quick turnaround here!

  1. I actually think it should be there, because we use that to switch between plain funnel and funnel conversion rate.
  2. Yeah that might be it, maybe better to use Ant's. Also from your screenshot not sure if you're on the latest version of the branch (as we dropped the Add filters button).
  3. Yup might be a good option, we could try it out. Regardless, I would at most keep it for the first filter row of each series.

@samwinslow
Copy link
Contributor Author

Cool, thanks! On issue 2 above (arrow styling) — that screenshot is what it looks like for me on master or with the feature flag turned off. Wondering if it looks the same for you or is also messed up. If so I'll switch the whole thing over to a different SVG icon.

@timgl timgl temporarily deployed to posthog-pr-4296 May 11, 2021 21:07 Inactive
@paolodamico
Copy link
Contributor

If it's okay, I'll merge this in and we can figure out these small details in a subsequent PR, as I've made a couple of follow-up PRs from this branch and so we can wrap all these for tomorrow.

@paolodamico paolodamico merged commit c7b71c4 into master May 12, 2021
@paolodamico paolodamico deleted the 4050-query-ui-optB-details branch May 12, 2021 02:34
@samwinslow
Copy link
Contributor Author

I am genuinely quite :confused: as to how you got the icon to display on only the select toggle... here's my attempt to do the same. Not bad necessarily, but not what was intended.

Screen Shot 2021-05-12 at 9 56 40 AM

@paolodamico
Copy link
Contributor

Yeah I relied on the optionLabelProp of Ant's Select, but can't seem to reproduce for the interval filter either. Perhaps it's just a matter of having the icon separate from the component? We just need to make sure it triggers the dropdown. Are you working on this in another PR? Happy to take a look.

@samwinslow samwinslow mentioned this pull request May 12, 2021
5 tasks
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