Combine events and actions into select box #2394
Conversation
|
This solves #2274 right? :) |
There was a problem hiding this comment.
Comments on UX,
- It'd be amazing if you could interact with the box with the keyboard (as we do with the command palette) to significantly speed up usage.
- Maybe consider changing to fuzzy searching as we do in the command palette? Probably it should just be a bit stricter, just to help with typos.
- I would maybe bold the event matches when you have a search term, in case you have a section collapsed that matches the event you're searching for.
- The "step" language seems confusing, "match group" or "matching conditions" could be better. We should definitely keep consistency on this too in the rest of the app.
- We can improve the hierarchy of elements in the action/event detail view to match the standardized components (e.g. the event name, would probably be the Level 3 subtitle element, as right now it's a bit confusing to have a mix of different sizes.
- Related to the above, I think it might be worth adding the events & actions icon to the details page, particularly now that we're using them to show the distinction more clearly.
- For events maybe add the "PostHog" indicator to autocapture/default events?
On the code side a minor thing, SelectBox seems a bit ambiguous / confusing. Did not deep dive into the code, but there doesn't seem to be any glaring issues.
|
Have solved most of the issues above. Haven't done the 'posthog logo' for posthog events yet as i don't want to balloon this PR. Also not sure what the subtitle 3 element is or how I get it, it's an h3, maybe we should style all h3's to be the same? cc @macobo would you mind giving this another look as Paolo is out? |
| export function SelectUnit({ | ||
| name, | ||
| dataSource, | ||
| dropdownLogic, |
There was a problem hiding this comment.
Q: Why is this a prop? Could find no other instances where we pass logics around this way besides this PR but I didn't look super hard.
There was a problem hiding this comment.
Hm take a look at PropertyFilter.js for example. It's a way of making sure the props for the logic are passed through correctly everywhere.
frontend/src/scenes/insights/ActionFilter/ActionFilterDropdown.scss
Outdated
Show resolved
Hide resolved
|
|
||
| const getSuggestions = (events: EventUsageType[]): EventUsageType[] => { | ||
| return events | ||
| .filter((event) => event.usage_count > 0) |
There was a problem hiding this comment.
Q: Should we even return these to the frontend?
There was a problem hiding this comment.
Do you mean the usage_count?
There was a problem hiding this comment.
No, events with usage_count == 0
| useEffect(() => { | ||
| if (selectedItemKey) { | ||
| const allSources = items.map((item) => item.dataSource).flat() | ||
| setSelectedItem(allSources.filter((item) => item.key === selectedItemKey)[0] || false) |
There was a problem hiding this comment.
why is the || false needed here?
There was a problem hiding this comment.
I'm actually not sure it is. I ran into this while developing but can't reproduce it anymore. I do still think it's safer, as the alternative is having a complete white-out
|
Note/thought for the future: We could have used feature flags here to develop this - put the new version behind a feature flag, roll it out and delete the old code afterwards. I think this has several upsides:
It does introduce the need to later come back and clean up the effectively dead code and make sure to turn the flag on - but this is a pain our users feel as well. All that said - no action required for this PR, just food for thought for future. |
|
Codewise this looks good. However missed a comment: #2394 (review) + the build failure might be relevant here. |
|
I had already fixed the toolbar issue (unless you're still seeing it?) and fixed build. |
Changes
Checklist