Bugs/improvements on better sessions

[paolodamico]

Some late feedback on #4840.

1. After I've opened a person page, the back button on the browser doesn't work, it keeps me in the person page but with some weird behavior going on. I'd expect to be able to get back to the person list if I click back.
2. Related to the above, if I click back to insights the person modal is closed when I get back.
3. The "matches" terminology seems a bit confusing. I don't know what I'm matching against. Maybe we should specify the exact event/action, or say something like "matches your trend query", not sure. CC @clarkus
   
4. The hover background color is very unintuitive, color should probably be a darker shade of yellow. You can try yellow 100 or darkening 10% the highlight color, but tagging @clarkus who will probably have a better idea.
   
5. I don't think expand all sessions should be a toggle but rather a button, because you can change the state of the sessions manually and then the toggle will show incorrect information. I would have here an "Expand all sessions"/"Collapse all sessions" button.
6. Something weird going on with the play all button, spacing is off.

[clarkus]

The "matches" terminology seems a bit confusing. I don't know what I'm matching against. Maybe we should specify the exact event/action, or say something like "matches your trend query", not sure. CC @clarkus

Good catch. We're matching against filters, so something to the effect of "N events match your event filters".

The hover background color is very unintuitive, color should probably be a darker shade of yellow. You can try yellow 100 or darkening 10% the highlight color, but tagging @clarkus who will probably have a better idea.

In this case, we're highlighting matched events within a session. It looks like the default hover treatment for a row is overriding that on hover? What problem are we trying to solve here? I think that the yellow is noticeable, but we might benefit from layering in some other affordance for the matched state. We could reuse the same events icon and use it as extra visual weight for identifying events that match. We could also add emphasis to the text values in the row. In the attached example I gave the event name bold weight to stand out even more.

[alexkim205]

It looks like the default hover treatment for a row is overriding that on hover?

Correct me if I'm wrong @paolodamico but I think this is the reason for the unintuitiveness? I think it makes sense to incorporate both of your feedback though! I pulled up the yellow hover color over a matched event just a bit and added some more emphasis. Let me know what you think of this-

Screen.Recording.2021-07-01.at.2.34.38.PM.mov

@clarkus Would it make more sense to have the event icon be a single square instead of a stack of squares?

[alexkim205]

Screen.Recording.2021-07-01.at.3.25.50.PM.mov

[alexkim205]

I think bugs 1-2 have to do with the way that kea-router actionsToUrl and urlToActions work. It looks like whenever that triggers we perform a silent history.push of the next url.

Me just pressing the back button after navigating to sessions page from the persons > events page. It takes a few clicks to get back to persons.

Screen.Recording.2021-07-01.at.3.35.53.PM.mov

This means that you need to click the back button several times to get back to the page you were actually one. @mariusandra do you have any pointers here on how to perform a correct back action when kea-router is doing this?

Edit: Might be related to decoupling state from url #4329.

[clarkus]

I see now. I think the direction to darken the yellow slightly on hover for highlighted rows makes sense. We would just limit that to highlighted rows only.

I keep thinking about this event icons at the event level and it feels off to me. We really just need a simple means of indicating a match regardless of types. The event icon with summary count at the session level still makes sense to me, but I wonder if a simple text mark / badge would be more meaningful. This would reiterate that highlight color (yellow) and give a simple visual label for indicating matches. What do you think?

[alexkim205]

I love it, implementing now

[alexkim205]

[mariusandra]

Hey @alexkim205 , this PR should fix the back button issue: #4966

[macobo]

What usecase does "expand all" serve? In a realistic scenario it just makes the whole page unusable due to the amount of noise. It also adds a ton of load to our servers to send megabytes of raw event data from the API.

Meta: It's worth keeping #4884 top of mind as doing sessions page fixes.

[alexkim205]

Hey @alexkim205 , this PR should fix the back button issue: #4966

Thanks @mariusandra! :verynice:

What usecase does "expand all" serve?

I think the expand all option is especially useful when it's used in tandem with the show only matches toggle, since there are times when you'd want to look at all the matching events in a given day regardless of which session it's in.

It also adds a ton of load to our servers to send megabytes of raw event data from the API.

I actually think we query for all events already upon first load of the sessions page, so expanding all sessions won't incur any more network costs than we already have. However I think this is only true for our Clickhouse users and not our SH users. We could also hide the expand all button behind a is_clickhouse_enabled check?

@macobo & @paolodamico what are your thoughts?

[clarkus]

I think the expand all option is especially useful when it's used in tandem with the show only matches toggle, since there are times when you'd want to look at all the matching events in a given day regardless of which session it's in.

I agree that the view when all sessions are expanded is pretty overwhelming. The show only option helps, but when the list of events is long it can quickly become unwieldy. I need to better understand how users are using this page. My assumption is that users are looking for the events in common across sessions so they can better understand how users are alike and different. Expanding all and only showing matches would allow a user to scroll through the page and see what events are recurring. Another way would be exposing the common events as suggested filters or even in some secondary area that describes the filtered set. There's a related conversation at https://github.com/PostHog/product-internal/issues/92#issuecomment-872602869. The takeaway there is "how do we expose common events from the list of sessions and make them more actionable and obvious".

Not really an answer, but it might get us closer to focusing on the core job to be done for users here. Thoughts?

[macobo]

However I think this is only true for our Clickhouse users and not our SH users. We could also hide the expand all button behind a is_clickhouse_enabled check?

Yup, Clickhouse loads all events currently and postgres does not. Both did initially - we turned it off to make sessions page usable on postgres, but similar performance overhead exists on clickhouse to a smaller extent - I am sure we'll move to all events being fetched from endpoint at some date.

[alexkim205]

Got it, thank you both for the context. I definitely agree there needs to be a way for users to view common events across sessions.

Given that this is still an open question design-wise, and not to mention the performance tradeoffs and future direction of the endpoints, I'll hide the expand all button behind a feature flag for the time being so that we can better understand use cases via user testing.

cc: side note but @paolodamico do you think this might be a good candidate for your upcoming user testing initiative?

[paolodamico]

This is looking awesome, thanks for the quick turnaround @alexkim205 & @clarkus. Very small feedback pieces,

1. I would consider keep the font weight normal as there's already enough emphasis from the yellow background. Thoughts?
2. Agreed on hiding the expand sessions button for now and testing (particularly because there's so many conversations going on that could make this a moot point). One thing to consider though is I really think this should be a single button (or even separate buttons, not a huge fan but could work) but not a radio group button. Reason is that there's no two single states of everything expanded or everything collapsed, you can be somewhere in the middle and therefore having a radio option could be misleading.