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

Improved session player navigation #2611

Merged
merged 12 commits into from
Dec 3, 2020
Merged

Improved session player navigation #2611

merged 12 commits into from
Dec 3, 2020

Conversation

paolodamico
Copy link
Contributor

Changes

  • Updates the way the session player is opened to be shown in the same page to provide a smoother experience.

    Default behavior. In place player
    image

    Cmd/Cntrl + click. Open in new tab
    image

  • Removes the legacy drawer (and related feature flag [full-page-player]).

  • Removes standalone session player page on /sessions/play?id={id} because it's redundant now with /sessions?sessionRecordingId={id}

  • Fixes some TS bugs in session recording.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@timgl timgl temporarily deployed to posthog-session-player--8y8jnq December 1, 2020 21:46 Inactive
@macobo
Copy link
Contributor

macobo commented Dec 2, 2020

Feedback:

  • ESC to exit is sweet! Nice feature!
  • It also doesn't reload the sessions page - good job!
  • It feels smooth to use!

Issues:

TypeError: Cannot read property 'timestamp' of undefined
    at vt.getDuration (index.modern.js:252)
    at $t (SessionsPlay.tsx:130)

@timgl timgl temporarily deployed to posthog-session-player--8y8jnq December 2, 2020 21:49 Inactive
@timgl timgl temporarily deployed to posthog-session-player--8y8jnq December 2, 2020 22:02 Inactive
@paolodamico
Copy link
Contributor Author

@macobo thanks for the thorough review, great catches! I've addressed all your feedback and greatly simplified the logic of sessionsTable, please feel free to review again

@timgl timgl temporarily deployed to posthog-session-player--8y8jnq December 2, 2020 22:09 Inactive
Copy link
Contributor

@macobo macobo left a comment

Choose a reason for hiding this comment

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

Awesome! I'd merge this and make a PR for the scroll bar issue but merging the other pr caused a merge conflict 😅

frontend/src/scenes/sessions/Sessions.scss Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-session-player--8y8jnq December 3, 2020 21:20 Inactive
@paolodamico paolodamico merged commit b5d4421 into master Dec 3, 2020
@paolodamico paolodamico deleted the session-player-nav branch December 3, 2020 21:47
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.

None yet

3 participants