-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
style: decouple Playlist and player. #27464
Conversation
It makes the layout more responsive + looks better on mobile
Size Change: +150 B (+0.01%) Total Size: 1.13 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated14 snapshot changes in total. 0 added, 14 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
…posthog into style/session-replay-ui
📸 UI snapshots have been updated14 snapshot changes in total. 0 added, 14 modified, 0 deleted:
Triggered by this commit. |
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.
we should probably have the section expanded by default in these stories so that we can see the row rendering is working
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.
i love that this changes brings this content back into view!
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.
ah.. i see. the split view hasn't applied to playlists here or in notebooks
@@ -51,7 +50,7 @@ | |||
|
|||
.SessionRecordingPlaylistHeightWrapper { | |||
// NOTE: Somewhat random way to offset the various headers and tabs above the playlist | |||
height: calc(100vh - 15rem); | |||
height: calc(100vh - 10rem); |
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.
dropping into both of our heads that it would be so cool to be able to remove this and just have this as a min-height and we fill the viewport otherwise
(although that can come... doesn't have to be in this PR 😊)
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.
this is great... just need to handle the playlist/notebook view too
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.
was sure i'd clicked approve already 🙈
📸 UI snapshots have been updated5 snapshot changes in total. 0 added, 5 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated11 snapshot changes in total. 0 added, 11 modified, 0 deleted:
Triggered by this commit. |
It makes the layout more responsive + looks better on mobile
Problem
Session replay page is not responsive as playlist and player are coupled.
Changes
Decoupled player and the list. Make them responsive.
Web
Before:
After:
Mobile
Before:
After: