-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
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) |
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. |
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. |
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..." |
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 |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
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
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. |
Ahaa! Thanks, makes a lot of sense, made the requested changes :) |
Would love to see this live. Will refrain from merging to give you the honours :D |
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. |
Makes sense, will change! |
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 😂 |
Problem
Hackathon PR!
Also fixes #9390
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?