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

disables page scroll when pressing spacebar #4204

Conversation

cassidypignatello
Copy link
Contributor

PR Checklist

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:

Fixes

Issue Number: #3870

What is the current behavior?

If playing a video and then navigating to a new page to cause the miniplayer to appear out of focus, pressing spacebar causes the video to pause and the page to scroll down in the background.

What is the new behavior?

Ensures that pressing spacebar only pauses the video on the miniplayer and doesn't cause the page to scroll.

Other information

@neb-b neb-b self-requested a review May 18, 2020 20:48
@lbry-bot lbry-bot assigned neb-b and unassigned neb-b May 18, 2020
Copy link

@neb-b neb-b left a comment

Choose a reason for hiding this comment

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

Two small comments.

};
window.addEventListener('keydown', handleKeyPress);
return () => window.removeEventListener('keydown', handleKeyPress);
});
Copy link

Choose a reason for hiding this comment

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

The second argument to this effect should be [] so it isn't re-ran on every render

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh good catch, thanks Sean! Updated

@@ -19,6 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
- Fix inconsistent relative-date string for claims, comments, etc. ([#4172](https://github.com/lbryio/lbry-desktop/pull/4172))
- Error opening certain files with special characters in name #2777 _community pr!_ ([#4161](https://github.com/lbryio/lbry-desktop/pull/4161))
- Comic-book file page shows download button first, and then viewer after download _community pr!_ ([#4161](https://github.com/lbryio/lbry-desktop/pull/4161))
- Prevents page from scrolling while pressing the spacebar when the miniplayer is out of focus _community pr!_ ([#4204](https://github.com/lbryio/lbry-desktop/pull/4204))
Copy link

Choose a reason for hiding this comment

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

Can you move this up to the unreleased section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like it's already under the unreleased section, did you want me to move it from Fixed to Changed maybe?

Copy link

Choose a reason for hiding this comment

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

Sorry. I didn't realize it was already in the correct section.

@lbry-bot lbry-bot assigned neb-b and unassigned neb-b May 18, 2020
@cassidypignatello
Copy link
Contributor Author

@seanyesmunt I just wanted to check in when you have a sec. I updated and pushed up the view.jsx file already, but didn't touch the CHANGELOG.md yet because it was already in the Unreleased section when I checked, but maybe I'm confused there. Let me know if you need any other changes, thanks!

@lbry-bot lbry-bot assigned neb-b and unassigned neb-b May 21, 2020
@neb-b
Copy link

neb-b commented May 21, 2020

Nope. Sorry we've been pretty busy with paid content on lbry.tv. I'll give this another pass/merge later tonight or tomorrow morning.

@cassidypignatello
Copy link
Contributor Author

Sounds good, no rush!

@neb-b
Copy link

neb-b commented May 25, 2020

@cassidypignatello Can you please rebase this? It looks like it includes a lot of changes that have already been merged to master.

@cassidypignatello cassidypignatello force-pushed the fix/3870-prevent-spacebar-from-scrolling-page branch from 8e8f4f1 to 788c7cf Compare May 25, 2020 16:19
@cassidypignatello
Copy link
Contributor Author

@seanyesmunt No problem, I just rebased this. Let me know if you need anything else!

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

2 participants