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

feat: Resizeable replayer inspector #18378

Merged
merged 15 commits into from
Jan 23, 2024
Merged

feat: Resizeable replayer inspector #18378

merged 15 commits into from
Jan 23, 2024

Conversation

benjackwhite
Copy link
Contributor

Problem

We have a nice generic resizer - lets make the player resizeable!

Changes

WIP

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

benjackwhite and others added 3 commits December 4, 2023 21:55
# Conflicts:
#	frontend/__snapshots__/lemon-ui-lemon-table--with-footer.png
#	frontend/src/layout/navigation-3000/components/Navbar.tsx
#	frontend/src/lib/components/Resizer/resizerLogic.ts
#	frontend/src/scenes/session-recordings/player/SessionRecordingPlayer.scss
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot posthog-bot removed the stale label Dec 5, 2023
@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@benjackwhite benjackwhite added waiting Prevents stale-bot from marking the PR as stale. and removed stale labels Jan 12, 2024
Copy link
Contributor

github-actions bot commented Jan 19, 2024

Size Change: 0 B

Total Size: 2 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 2 MB

compressed-size-action

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

looks good to me

Copy link
Member

Choose a reason for hiding this comment

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

commenting here so we can thread :)

it felt like it should always be resizeable. it loaded I tried to grab it, I couldn't, then I collapsed a side menu and now I could grab it and resize it... confusing.

I think it's ok to have different min/max sizes based on screensize, but should always be resizable on desktop IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point. I was leaving the existing behaviour where it was always expanded when the container was large enough in place. That probably matters a lot less given it's resizeable.

Think the happy middle here is to always make it expanded when there is enough space, but to not disable resizing just because the screen is wide enough. Will implement that

Copy link
Contributor Author

@benjackwhite benjackwhite Jan 23, 2024

Choose a reason for hiding this comment

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

Chiming in! I think it should be always moveable / collapsable.

  1. Small screen: it is collapsed by default. You click it to open it, can resize it but clicking on the player (i.e. the bit behind it, collapses it.
  2. Big enough screen: Same behaviour except it doesn't float over the player, rather it is just next to it and you can resize within sensible bounds and collapse

In both cases I think it needs a button to hide as welll (not just the resizer)

@daibhin daibhin marked this pull request as ready for review January 23, 2024 09:21
@daibhin daibhin merged commit 253c8b5 into master Jan 23, 2024
100 checks passed
@daibhin daibhin deleted the feat/inspector-resizing branch January 23, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Prevents stale-bot from marking the PR as stale.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants