New filtering experience #5189

mariusandra · 2021-07-18T23:24:48Z

Changes

Everything here is behind feature flag 4267-taxonomic-property-filter

Continuation of #5098 - however considering how much the original PR and this one have diverged, I'd just merge this one into master instead.

TODO:

I'd like to merge this in before tackling the remaining issues.

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
New/changed UI is decent on smartphones (viewport width around 360px)

samwinslow · 2021-07-19T17:10:38Z

frontend/src/lib/components/PropertyFilters/utils.ts

+    return filterType && filterType in propertyFilterMapping ? propertyFilterMapping[filterType] : undefined
+}
+
+export function taxonomicFilterTypeToPropertyFilterType(filterType?: TaxonomicFilterGroupType): string | undefined {


Minor stylistic question: Why have a function do this every time with find, rather than reverse the mapping of propertyFilterMapping once and just use object attributes directly? Wouldn't Object.fromEntries(Object.entries(propertyFilterMapping).map(([ k, v ]) => [v, k])) accomplish that?

It would. Though this is running in onChange handlers only, so the performance boost will be pretty tiny. I'll fix it in a follow up PR. Hope to get this in its current state in front of as many eyeballs as quickly as possible.

samwinslow · 2021-07-19T17:13:45Z

frontend/src/lib/components/TaxonomicFilter/InfiniteList.tsx

+    const [referenceElement, setReferenceElement] = useState<HTMLDivElement | null>(null)
+    const [popperElement, setPopperElement] = useState<HTMLDivElement | null>(null)
+
+    const { styles, attributes } = usePopper(referenceElement, popperElement, {


Cool! Going forward will react-popper replace antd Tooltips?

I hope so! Since react-popper itself is just a hook, we need to build the actual popup component and its interactions with the rest of the app ourselves. This can get tricky. For example I already had to include some funky code to make sure that we can have multiple popups on the page at the same time, without interfering in each other's onClickOutside logic. There will surely be more stuff like this to come.

clarkus · 2021-07-19T17:34:31Z

Attached is a basic empty state focused on resetting the type-ahead field to its initial state. Clicking the button would reset the type-ahead input to have no value, and would reload the picker with its default parameters.

Edit - just saw that we have a pretty good error state in there now. I think that could be enough to release as we mostly want to inform of the "no matches" case. Adding in the action to clear the parameters would be a "nice to have" change depending on our timeline.

clarkus · 2021-07-19T18:02:24Z

After testing this for a while I think it's a huge improvement. I don't have any feedback I would consider blocking at this point, but I did have some longer-term things to think about.

"All" tab
I was working on some similar designs that included an "all" category. This was mostly to ensure that users who load the filter component and type in a keyword see something in the initial list without have to navigate tabs. I don't think it's critical for this portion of work, but it would be something to consider longer term as a way to show a matching results with less effort.

Consistent "show filter" placement
The place where we trigger the filtering component varies across insights. As part of this change, it'd be great to align funnels with the other insight types that support filtering Placing the "show filter" action adjacent to the thing it filters would be a good spot.

clarkus · 2021-07-19T18:16:23Z

@mariusandra One potential issue I just saw - we're skipping over the operator field when automatically changing focus. Right now I can select a filter property using the keyboard, hit enter and the value field focuses automatically. This puts us into an is equal to scenario by default. Is that intentional? Is there any reason we'd want a user to select an operator before prompting for a value?

mariusandra · 2021-07-19T19:42:24Z

I rebased with master (lost the commit history along the way 🤷, ask if you want it) and will stop adding things to this PR unless anyone finds critical bugs. Everything will still be behind a feature flag and I'd like to get this in front of as many internal eyeballs as possible, as quickly as possible.

Regarding feedback:

The empty state is the default antd Empty compnent. Happy to make it something nicer, though IMO it's also fine as is, for now. The best solution would be to design our own empty state widget, featuring some joy sparkingly confused hedgehogs.
Bug to fix: The empty state is weird if you have 0 of something and haven't searched, it'll say No results for ""
+1 to the "clear parameters" button
I'd love an "All" tab, yet I'm not sure how to implement it. Would it be like it is now: a tree view, or basically just combining the 4 tabs into one tab? Would everything be mixed up underneath each other? Randomly? Alphabetically with tags for categories? By what the algorthm decides is best for you? Top 5 most popular items from each category?
Regarding "Consistent "show filter" placement", I think you took the screenshot on Heroku where the new funnels weren't enabled? It looks different on app with the feature flag enabled:
Regarding the focus: yes, this is something that needs to be improved! I'm not sure if it's a bad thing to focus on the value though. You can shift+tab back to the operator. Though what do I know.
I'm not happy with the "PropertyValue" widget. I think this selection behaviour makes sense if you want to select multiple things, but now it just looks odd (and is even broken!):

... though I didn't touch this dropdown at all in this PR. I'll gladly improve this in a follow up.

clarkus · 2021-07-19T20:10:40Z

4. I'd love an "All" tab, yet I'm not sure how to implement it. Would it be like it is now: a tree view, or basically just combining the 4 tabs into one tab? Would everything be mixed up underneath each other? Randomly? Alphabetically with tags for categories? By what `the algorthm` decides is best for you? Top 5 most popular items from each category?

Good questions. I was thinking it'd just be the tab groupps stacked in the same order, but then that begs the question "why do we have tabs?" Maybe we should go with this and see how users respond to the changes.

5. Regarding "Consistent "show filter" placement", I think you took the screenshot on Heroku where the new funnels weren't enabled? It looks different on `app` with the feature flag enabled:

👍

6. Regarding the focus: yes, this is something that needs to be improved! I'm not sure if it's a bad thing to focus on the value though. You can shift+tab back to the operator. Though what do I know.

Yeah it's just weird that we skip it in the order when we don't do that anywhere else. Jumping to the value field could be ok if the "is equal to" operator is our most common use case. The opposite case could also be true. Users could be annoyed if they always have to navigate through the operator to set a value.

7. I'm not happy with the "PropertyValue"  widget. I think this selection behaviour makes sense if you want to select multiple things, but now it just looks odd (and is even broken!):
   ![2021-07-19 21 41 10](https://user-images.githubusercontent.com/53387/126217606-9241195f-2a45-4900-a566-9c124a3964ee.gif)
   ... though I didn't touch this dropdown at all in this PR. I'll gladly improve this in a follow up.

👍 I am working on some ideas for this. I'll try to get that ready to inform a follow up.

paolodamico

lgtm! UX-wise I only found a couple of bugs,

The bug when you select a property value the other options disappear is still happening.
There's some weird behavior when selecting values (see recording).
Large values are not handled well.

4. Not convinced on the "Elements" tab, think that can be confusing. Any ideas @clarkus ?

paolodamico · 2021-07-19T20:04:44Z

frontend/src/scenes/trends/personsModalLogic.ts

@@ -120,7 +120,6 @@ export const personsModalLogic = kea<personsModalLogicType<PersonModalParams>>({
            }
        },
        loadPeople: async ({ peopleParams }, breakpoint) => {
-            actions.setPeopleLoading(true)


This was actually just re-introduced in #5190 to fix loading. Is this a bug? CC @liyiy

Nice catch! Thanks! :)

paolodamico · 2021-07-19T20:09:13Z

frontend/src/scenes/paths/pathsLogic.ts

-            () => [propertyDefinitionsModel.selectors.loaded],
-            (propertiesLoaded): boolean => !propertiesLoaded,
+            () => [featureFlagLogic.selectors.featureFlags, propertyDefinitionsModel.selectors.loaded],
+            (featureFlags, loaded) => !featureFlags[FEATURE_FLAGS.TAXONOMIC_PROPERTY_FILTER] && !loaded,


Nit: Should the selector have a return type for clarity?

Return type? Not sure * loaded and * loading kind of imply a yes/no state IMO... and typegen also doesn't need an explicit type here, so 🤷

paolodamico · 2021-07-19T20:12:48Z

frontend/src/lib/components/PropertyKeyInfo.tsx

@@ -512,6 +512,53 @@ interface PropertyKeyInfoInterface {
    ellipsis?: boolean
 }

+export function PropertyKeyTitle({ data }: { data: KeyMapping }): JSX.Element {


Nit: Does this need to be exported?

I need to import it in InfiniteList. This all should get a good refactor though, including replacing the default popup library. Then I'd merge the PropertyKeyTitle and PropertyKeyDescription components as well.

mariusandra · 2021-07-19T21:57:51Z

Perfect! Thanks for the review and catching that bug reversal! I probably thought I was in a loader instead of being in a listener :/. I'll wait for the tests to pass and then merge this in. The the flag is enabled for the team.

Tomorrow's focus: improving the property value select and fixing all other feedback... then asking for more of it.

timgl temporarily deployed to posthog-pr-5189 July 18, 2021 23:26 Inactive

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 06:50 Inactive

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 08:31 Inactive

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 09:12 Inactive

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 11:10 Inactive

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 11:19 Inactive

mariusandra mentioned this pull request Jul 19, 2021

Refactor taxonomic property filter logic #5098

Merged

6 tasks

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 12:51 Inactive

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 12:57 Inactive

mariusandra requested a review from paolodamico July 19, 2021 13:02

mariusandra marked this pull request as ready for review July 19, 2021 13:02

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 13:04 Inactive

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 14:19 Inactive

mariusandra requested a review from samwinslow July 19, 2021 14:41

mariusandra changed the title ~~New filtering experience~~ ~~New~~Nail filtering experience Jul 19, 2021

mariusandra changed the title ~~~~New~~Nail filtering experience~~ New, néé, Nail filtering experience Jul 19, 2021

mariusandra changed the title ~~New, néé, Nail filtering experience~~ New filtering experience Jul 19, 2021

samwinslow reviewed Jul 19, 2021

View reviewed changes

the new filtering experience

67b54cc

mariusandra force-pushed the the-posthog-filtering-experience branch from c5d6924 to 67b54cc Compare July 19, 2021 19:08

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 19:08 Inactive

Merge branch 'master' into the-posthog-filtering-experience

0a97b20

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 19:09 Inactive

clarkus mentioned this pull request Jul 19, 2021

EPIC: Filters UX 2.0 #2760

Closed

paolodamico approved these changes Jul 19, 2021

View reviewed changes

revert bug

29191e9

mariusandra temporarily deployed to posthog-pr-5189 July 19, 2021 21:55 Inactive

mariusandra enabled auto-merge (squash) July 19, 2021 21:57

mariusandra merged commit 5245ee6 into master Jul 19, 2021

mariusandra deleted the the-posthog-filtering-experience branch July 19, 2021 22:28

mariusandra mentioned this pull request Jul 23, 2021

New filter improvements #5314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New filtering experience #5189

New filtering experience #5189

mariusandra commented Jul 18, 2021 •

edited

Loading

samwinslow Jul 19, 2021

mariusandra Jul 19, 2021

samwinslow Jul 19, 2021

mariusandra Jul 19, 2021

clarkus commented Jul 19, 2021 •

edited

Loading

clarkus commented Jul 19, 2021

clarkus commented Jul 19, 2021

mariusandra commented Jul 19, 2021

clarkus commented Jul 19, 2021

paolodamico left a comment

paolodamico Jul 19, 2021

mariusandra Jul 19, 2021

paolodamico Jul 19, 2021

mariusandra Jul 19, 2021

paolodamico Jul 19, 2021

mariusandra Jul 19, 2021

mariusandra commented Jul 19, 2021

New filtering experience #5189

New filtering experience #5189

Conversation

mariusandra commented Jul 18, 2021 • edited Loading

Changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarkus commented Jul 19, 2021 • edited Loading

clarkus commented Jul 19, 2021

clarkus commented Jul 19, 2021

mariusandra commented Jul 19, 2021

clarkus commented Jul 19, 2021

paolodamico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra commented Jul 19, 2021

mariusandra commented Jul 18, 2021 •

edited

Loading

clarkus commented Jul 19, 2021 •

edited

Loading