Rework `SessionsPlayerButton` and clean up Sessions UI #5745

Twixes · 2021-08-25T18:27:06Z

Changes

This is a prerequisite for #5664 that reworks SessionsPlayerButton into something that will also be useful in the persons modal. Based on a design by @clarkus (#5664 (comment)), though a bit more lightweight than there at the moment – can always adjust any styles.
Also cleaned up nearby Sessions UI.

Before	After

clarkus

This change looks good to me. I missed this in the earlier design, but I wonder if the color is enough to communicate previously watched recordings. We could include a tooltip on hover that clarifies the meaning of each icon state.

alexkim205 · 2021-08-25T21:14:09Z

frontend/src/lib/components/Popup/Popup.tsx

@@ -82,7 +84,7 @@ export function Popup({
            {visible
                ? ReactDOM.createPortal(
                      <div
-                          className="popper-tooltip"


Nit: There's a handy library called clsx that can make this shorter.

className={clsx('popper-tooltip', className)}

That's great, thanks

frontend/src/scenes/sessions/SessionRecordingsButton.tsx

alexkim205 · 2021-08-25T21:19:59Z

posthog/queries/sessions/test/test_session_recording.py

+                for session_recording_package in session_recording_results:
+                    for session_recording in session_recording_package:
+                        del session_recording["start_time"]
+                        del session_recording["end_time"]


Would it be worth it to use a new snapshot so that expected reflects these new parameters?

It's kind of bit menial to add them (it's not really a snapshot, just values in self.assert* calls) due to quirks in datetime comparisons (I had some problems with tzinfo when I tried to do this before going for del) and I don't think this is important to getting this in, so I'll leave this with just a TODO for now.

alexkim205 · 2021-08-25T21:22:56Z

Nice! lgtm overall but left some feedback inline and below.

Are these two durations supposed to be the same? It's confusing that 1min is rounded up from 18s in this case.

macobo · 2021-08-26T05:52:21Z

Q: In an ideal world most sessions would not have a dropdown. Do we show one even for those (as indicated by the screenshot?) If so - why? IMO This makes the "majority" case of navigation much slower - you expect most sessions (especially in modals) to have one recording only.

Also this kills a feature - being able to distinguish whether a user has seen a recording or not. This is something that's quite vital for session recordings - you don't want to watch one twice.

As-is this change would make my usage of session recordings more painful.

Note also #4884 - this quirk is something I think is temporary and we'll hopefully do away with soon.

cc @clarkus

Twixes · 2021-08-26T15:07:31Z

@alexkim205 Thanks for the review. As for durations, yes, they should definitely be different, as a PostHog session is detached from a session recording, that is a single session can have multiple session recordings, a single one, or none. So it's calculated separately based on PostHog events.

@macobo I see what you mean. I added a play icon to the button too, which is blue if there's an unwatched recording under it, and gray if all have already been watched. This should improve scannability.

Initially I wanted to make it play without a dropdown in case of a single recording, but I ran into a weird UX mismatch, where you'd be able to see the details of recordings when there were multiple of them (thanks to the dropdown), but you'd be clicking blindly in case of a single recording – no place to show the timestamp.
Alternatively I tried showing the dropdown on hover instead of on click, but that was super tricky with the way the popper component uses a ref + would potentially be annoying when moving the cursor over the column.
Hence I went for the dropdown-on-click-always approach.
Happy to hear what you think about this and iterate though (best in a later PR though). @clarkus @macobo

Updated button:

clarkus · 2021-08-26T15:20:58Z

@macobo I think we can maintain the watched / unwatched treatment given my previous comment. It's essentially how it works now with different color coding and additional tooltips to translate what each icon color means. Is there more to it, or would that be sufficient to communicate watched / unwatched?

I do think we should trigger dropdowns with clicks as much as possible. That's pretty consistent across all our dropdowns.

Twixes · 2021-08-26T15:24:17Z

Also added a 500+ ms hover tooltip to the play icon for individual recordings :)

macobo · 2021-08-26T15:26:40Z

@clarkus

@macobo I think we can maintain the watched / unwatched treatment given my previous comment.

Problem being you need to open a dropdown to see it in the first place. Given that these lists are supposed to get large, we have made a process that used to be purely visual into a clicking fest.

@Twixes

@macobo I see what you mean. I added a play icon to the button too, which is blue if there's an unwatched recording under it, and gray if all have already been watched. This should improve scannability.

I think so too. Didn't check out the code but looks much better.

Initially I wanted to make it play without a dropdown in case of a single recording, but I ran into a weird UX mismatch, where you'd be able to see the details of recordings when there were multiple of them (thanks to the dropdown), but you'd be clicking blindly in case of a single recording – no place to show the timestamp.

Very good point IMO re the design.

All-in-all it brings up the question of what problem we're solving and why?

For Sessions page specifically, after #4884 has been resolved we will have 1-1 matching. For people modal, I'd expect similar - one session recording per person 98% of the time (since it's based on user navigating around, how often do you use two browsers at once?)

What are we gaining by simplifying the 2% case?

clarkus · 2021-08-26T15:33:11Z

Problem being you need to open a dropdown to see it in the first place. Given that these lists are supposed to get large, we have made a process that used to be purely visual into a clicking fest.

True, but we've also added extra information to communicate the freshness of recordings and their duration. It's another click but you can have more info to indicate which session might be worth watching. We are going to hit some uppperbound for scale here at some point. We could adjust to showing recordings inline with an overflow option that links off to a full list of recordings for the user, but I'm not sure we'd be able to include the other information in that case.

What are we gaining by simplifying the 2% case?

I think we're making it better until #4884 is completed. Once we have more confidence in a 1:1 match, we can adjust here. Given how things are working now, this feels like a better experience until we're in a more ideal state.

paolodamico

Sorry for the delay here. Quick posthumous review (UX/product, not code) based on PostHog Cloud to improve in a subsequent PR,

There's a weird flicker with scrollbars which makes interacting with the page really difficult.
Probably related to the above, the tooltip in the Recordings column header is no longer showing.
We should just allow direct clicking on the button if there's only one recording. No point in showing the dropdown. This way it can also be clear at a glance to users when a session has multiple recordings.

clarkus · 2021-08-26T18:40:26Z

Oh I thought it was set to only show a drop down in the case of multiple recordings. Otherwise it's a button.

Twixes · 2021-08-26T21:55:21Z

Couldn't reproduce the flickering, but there is a problem with sizing the last column in ResizableTable. Kind of outside the scope of this PR, but may be good to take a look at it later.
I see the point with the single recording case, put out a proposal how to handle it: #5757

Twixes added 6 commits August 24, 2021 21:28

Refactor PersonModal and clean related components up a little

c025371

Improve footer button conditions

5ce7097

Remove unused vars

1bd5a93

Clean up session recordings table

d0bebfc

Rework SessionsPlayerButton

e1c76f3

Fix color of play button for viewed recordings

704b224

Twixes requested review from clarkus, paolodamico and alexkim205 August 25, 2021 18:27

timgl temporarily deployed to posthog-pr-5745 August 25, 2021 18:29 Inactive

Fix typing issue

ae0184e

Twixes temporarily deployed to posthog-pr-5745 August 25, 2021 18:32 Inactive

clarkus approved these changes Aug 25, 2021

View reviewed changes

Fix tests

a693139

Twixes temporarily deployed to posthog-pr-5745 August 25, 2021 19:21 Inactive

Update Sessions.cy-spec.js

d9e2c83

Twixes deployed to posthog-pr-5745 August 25, 2021 19:46 View deployment

alexkim205 reviewed Aug 25, 2021

View reviewed changes

Base automatically changed from persons-modal-refactor to master August 26, 2021 11:44

Twixes added 2 commits August 26, 2021 14:44

Merge branch 'master' into persons-modal-recordings

007fd35

Address feedback

bc33c44

Twixes temporarily deployed to posthog-pr-5745 August 26, 2021 15:00 Inactive

Simplify button styles

f20a262

Add tooltip

8ff2e82

Twixes merged commit 84dc2b0 into master Aug 26, 2021

Twixes deleted the persons-modal-recordings branch August 26, 2021 17:17

paolodamico reviewed Aug 26, 2021

View reviewed changes

Twixes mentioned this pull request Aug 26, 2021

Adjust SessionRecordingsButton behavior for a single recording #5757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework `SessionsPlayerButton` and clean up Sessions UI #5745

Rework `SessionsPlayerButton` and clean up Sessions UI #5745

Twixes commented Aug 25, 2021

clarkus left a comment

alexkim205 Aug 25, 2021

Twixes Aug 26, 2021

alexkim205 Aug 25, 2021

Twixes Aug 26, 2021

alexkim205 commented Aug 25, 2021

macobo commented Aug 26, 2021 •

edited

Loading

Twixes commented Aug 26, 2021 •

edited

Loading

clarkus commented Aug 26, 2021

Twixes commented Aug 26, 2021

macobo commented Aug 26, 2021 •

edited

Loading

clarkus commented Aug 26, 2021 •

edited

Loading

paolodamico left a comment

clarkus commented Aug 26, 2021

Twixes commented Aug 26, 2021

Rework SessionsPlayerButton and clean up Sessions UI #5745

Rework SessionsPlayerButton and clean up Sessions UI #5745

Conversation

Twixes commented Aug 25, 2021

Changes

clarkus left a comment

Choose a reason for hiding this comment

alexkim205 Aug 25, 2021

Choose a reason for hiding this comment

Twixes Aug 26, 2021

Choose a reason for hiding this comment

alexkim205 Aug 25, 2021

Choose a reason for hiding this comment

Twixes Aug 26, 2021

Choose a reason for hiding this comment

alexkim205 commented Aug 25, 2021

macobo commented Aug 26, 2021 • edited Loading

Twixes commented Aug 26, 2021 • edited Loading

clarkus commented Aug 26, 2021

Twixes commented Aug 26, 2021

macobo commented Aug 26, 2021 • edited Loading

clarkus commented Aug 26, 2021 • edited Loading

paolodamico left a comment

Choose a reason for hiding this comment

clarkus commented Aug 26, 2021

Twixes commented Aug 26, 2021

Rework `SessionsPlayerButton` and clean up Sessions UI #5745

Rework `SessionsPlayerButton` and clean up Sessions UI #5745

macobo commented Aug 26, 2021 •

edited

Loading

Twixes commented Aug 26, 2021 •

edited

Loading

macobo commented Aug 26, 2021 •

edited

Loading

clarkus commented Aug 26, 2021 •

edited

Loading