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(replays): Play / Pause Spacebar Handler #38054

Closed
wants to merge 2 commits into from
Closed

feat(replays): Play / Pause Spacebar Handler #38054

wants to merge 2 commits into from

Conversation

danecando
Copy link
Contributor

Changes

  • ReplayPlayer registers a keydown event listener for the space key that toggles play/pause state

Closes #37934

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@danecando danecando requested a review from a team as a code owner August 19, 2022 16:44
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 19, 2022
Copy link
Member

@ryan953 ryan953 left a comment

Choose a reason for hiding this comment

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

This is preventing me from typing a space into the 'Search console logs...' text input

@danecando
Copy link
Contributor Author

danecando commented Aug 22, 2022

Ryan mentioned possibly handling some other keyboard shortcuts and considering any edge cases like the one in my initial commit.

Looking at the YouTube docs for example I can see a few other shortcuts we might find useful. https://support.google.com/youtube/answer/7631406?hl=en

  • left and right arrow for seeking
  • f for activating full screen
  • could also add the speed up slow down shortcuts > <

Aside from that I think registering them on the Player component makes sense. Will just need to add more handlers for the different keys.

As for the original issue where I prevented the space bar from working on the input. The code doesn't even really need the preventDefault - I just found the scrolling behavior on spacebar to be a little bit annoying. I added a quick snippet for ignoring shortcuts when text inputs are active as an alternative to removing it.

cc @jesus4497 @ryan953 @billyvg

@ryan953
Copy link
Member

ryan953 commented Aug 31, 2022

There's no spec for this yet, the ticket #37934 is currently empty. So i'm going to close this PR and we can figure out what the actual task is in the ticket, then maybe resurrect this and make whatever changes need to happen later.

@ryan953 ryan953 closed this Aug 31, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard event listeners for replay playback (aka: shortcuts)
2 participants