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

feat(Universal-search): Search for literally anything in PostHog #9299

Merged
merged 22 commits into from
Apr 19, 2022

Conversation

neilkakkar
Copy link
Contributor

@neilkakkar neilkakkar commented Mar 30, 2022

Problem

Hackathon PR!

Also fixes #9390

2022-04-12 15 17 36

We reused most of taxonomic filter stuff to create this. Opted to not weave all of this into existing taxonomicFilter because this allowed us to move quicker, and things were different enough that it didn't make sense to try and put all the same conditionals in here.

Will now check how much of an effort it is to reconcile back with the taxonomic filter 👀 , before merging.

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@neilkakkar neilkakkar requested a review from timgl March 30, 2022 17:08
@neilkakkar
Copy link
Contributor Author

cc: @clarkus , if you had any design considerations to raise here? (initial launch would be gated behind a FF just for our team, so no rush)

@clarkus
Copy link
Contributor

clarkus commented Apr 12, 2022

Given that it's global, does it makes sense to show a single category of results by default? A combined list of all matching results sorted by best match would make it so that there's potentially less effort to see the thing you're looking for. The categorical grouping of results works well for filtering because it's most likely you're looking for an event in that case. In a global position, it's a bit more problematic to bias a single category over others. Is it feasible to show an "all" category?

Secondary to that, showing meta data or other attributes that help identify a result would be good - things like the creator of the thing, a description, tags, etc. It's a broader set, so there might be more overlap between results, so more data could help a user identify the thing they're looking for.

The width of the control is a lot - I know we're making room for a big rich placeholder, but I'm not sure we need to be explicit here. It could just say "search" given that it's global and searches everything. I just want to think about other contexts where there is less viewport width available - don't want to outcompete or displace other stuff.

@clarkus
Copy link
Contributor

clarkus commented Apr 13, 2022

Another thought - search can be used to get back to things you're looked at recently. This could be a great feature for showing recently viewed or frequently viewed items so it's easy to get back to where you were.

@mariusandra
Copy link
Collaborator

Flyby: looks really cool!

Having most relevant results in one list would be nice, but I understand that it's technically tricky, and the current work is a prerequisite anyway, so I'm happy with it how it is --> it's already so so much better than the current command palette.

Adding a "recents" tab, and the other suggestions by @clarkus make sense. The placeholder could also just say "Search..."

@neilkakkar neilkakkar requested a review from mariusandra April 14, 2022 10:10
@neilkakkar neilkakkar marked this pull request as ready for review April 14, 2022 10:11
@neilkakkar
Copy link
Contributor Author

Agree on both points, makes sense!

Chris, when you say a single list, do you mean something like sublists each with their own header telling what type of thing you're looking at, in an infinite scroll? Might work, but unsire how we'll handle, say, thousands of persons / events in this list? they'd have to be at the end to not drown out the other small lists?

Have gated this behind a feature flag, I also want to check out how long this search takes to populate on our instance, vs local where its blazing fast, hence pushing to release only for us without all the design improvements in this PR.

@@ -67,7 +70,17 @@ export const infiniteListLogic = kea<infiniteListLogicType>({
key: (props) => `${props.taxonomicFilterLogicKey}-${props.listGroupType}`,

connect: (props: InfiniteListLogicProps) => ({
values: [taxonomicFilterLogic(props), ['searchQuery', 'value', 'groupType', 'taxonomicGroups']],
// TODO: had to connect FF to get the model loaded for filtering
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mariusandra specifically here, I think this is fine to do, but wanted another set of eyes here!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, this works... but my main concern is that this lib/component now pulls in a lot of deeply nested logics just to display a universal dropdown. 🤔

Pulling feature flagLogic is probably fine, but I'm not 100% convinced of the other ones. Perhaps they could be passed in as props to the logic, like appScenes are originally passed to sceneLogic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I actually don't need these at all in here, just need to make sure they're mounted, so I moved this to UniversalSearchPopup instead, keeping this component clean and blind.

c647b6a

Is there a better way to ensure a logic is mounted? I don't face this problem with others because they're models that are mounted on app start

@clarkus
Copy link
Contributor

clarkus commented Apr 14, 2022

Chris, when you say a single list, do you mean something like sublists each with their own header telling what type of thing you're looking at, in an infinite scroll? Might work, but unsire how we'll handle, say, thousands of persons / events in this list? they'd have to be at the end to not drown out the other small lists?

I mean a single combined list of results sorted by best match and not by any other categorical or grouping criteria. If a user enters "Foo" we'd show the top N results that are the best match for "Foo" in a single list of results. It just makes it so you don't have to know the type you're looking for and can scroll to see the best matches. It'd give more a sense of immediacy and speed without having to have a bunch of context. It's definitely a longer term thing as I know this kind of aggregation is difficult.

@neilkakkar
Copy link
Contributor Author

Ahaa! Thanks, makes a lot of sense, made the requested changes :)

@mariusandra
Copy link
Collaborator

Would love to see this live. Will refrain from merging to give you the honours :D

@neilkakkar neilkakkar merged commit a20d57a into master Apr 19, 2022
@neilkakkar neilkakkar deleted the universal-search-fe branch April 19, 2022 09:26
@clarkus
Copy link
Contributor

clarkus commented Apr 19, 2022

Just a quick callout - we didn't actually change the placeholder value for the search so it is still pretty large. I don't think it needs to be that wide and I'm not sure the verbose tooltip is helpful. It could just say "search" and be just as effective.

@neilkakkar
Copy link
Contributor Author

Makes sense, will change!

@mariusandra
Copy link
Collaborator

The persons search isn't giving me much:
image

Otherwise, this is very cool. Now it should also open on CMD+K :D.

@neilkakkar
Copy link
Contributor Author

Yes indeed, noticed that. The same problem exists for searching on Persons page. That search query is terrible.

In other news, try CMD+S, because I wasn't sure what to do with the palette 😂

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.

Feature Request: Search, for Orgs
4 participants