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

Prevent default event for shortcut #2696

Closed
wants to merge 1 commit into from

Conversation

Aiz0
Copy link
Contributor

@Aiz0 Aiz0 commented Oct 9, 2022

Prevent default event for shortcut

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

no related issue

Description

Adds event.preventDefault() to the shortcuts added in #2689.
This fixes an issue where pressing shortcut also caused sidebar to expand/close when hamburger button was last element clicked.

Screenshots

no visible difference

Testing

  1. Go to Subscription page
  2. Press the hamburger menu to expand/close the sidebar.
  3. Press the any alpha character key such as the A key
  4. See that the menu expands or closes. (Is this intended?)
  5. Press the R key
  6. See that nothing happens to the menu.

Desktop

  • OS: Arch Linux
  • OS Version: 5.19.13-arch1-1
  • FreeTube version: 0.17.1

Additional context

I don't really know why the sidebar behaves that way or if it's something to be fixed.

Fixes an issue where pressing it caused sidebar to expand/close when hamburger button was last element clicked
@PrestonN PrestonN enabled auto-merge (squash) October 9, 2022 23:02
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 9, 2022
@Aiz0
Copy link
Contributor Author

Aiz0 commented Oct 9, 2022

event.preventDefault() seems to be missing from some video player shortcuts causing the same behavior.

Should I add it to those as well or should we consider some other solution?

@Aiz0
Copy link
Contributor Author

Aiz0 commented Oct 9, 2022

This line seems to be the issue

@keypress="toggleSideNav"

It seems to read all keypressess.
Using @keydown.enter.prevent would probably be preferable.

I'll open a new PR to address that.
preventing default events for these keys will no longer be necessary as far as I know.

@Aiz0
Copy link
Contributor Author

Aiz0 commented Oct 10, 2022

Closing this because i opened the other one.
I can reopen this if we do want to add event.preventDefault() to the keybinds.

@Aiz0 Aiz0 closed this Oct 10, 2022
auto-merge was automatically disabled October 10, 2022 00:31

Pull request was closed

@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 10, 2022
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.

1 participant