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.

Sign up for GitHub

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

Jump to bottom

Keyboard shortcut modal #6306

Open
wants to merge 18 commits into
base: development
Choose a base branch
from
Open

Keyboard shortcut modal #6306

wants to merge 18 commits into from
445 −12

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Dec 8, 2024 •
edited
Loading

Keyboard shortcut modal

Pull Request Type

  • Feature Implementation

Related issue

closes #2822

Description

  • Adds Ctrl Z Shift ? keyboard shortcut (same as YT) (for non- mobile device sizes only) button at the top of the settings page that opens/closes a modal containing all of the keyboard shortcuts available in FreeTube
  • Also tweaks ft-prompt logic to check for ft-icon-buttons as well, not just ft-buttons

Screenshots

Screenshot_20241207_185233
Screenshot_20241207_185206
Screenshot_20241216_185105

Testing

  • Test Shift ? opens and closes the new modal from anywhere in the app
  • Test that close button is focused properly on open, even when another modal is already open
  • Check that modal content appears fine with different simulated device sizes
  • Play a video in fullscreen and/or fullwindow, press Shift ?, and ensure that FS and/or FW are successfully exited such that the keyboard shortcut modal is now visible

Desktop

  • OS: OpenSUSE
  • OS Version: TW

Additional context

  • I tried seeing if there was a simple programmatic way to check if the user is using a keyboard to determine whether to hide the button in Settings, but most solutions are messy and/or leave ample room for inaccuracy.

@kommunarr kommunarr added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 8, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 8, 2024 01:33
@absidue
Copy link
Member

absidue commented Dec 8, 2024

I chose Ctrl Z as a placeholder but ended up sticking with it. I'm fine with switching this to something else if there's a good reason to do so.

CTRL Z is the standard keyboard shortcut for undo on Windows. We should probably use the same shortcut that YouTube does.

absidue
absidue reviewed Dec 8, 2024
View reviewed changes
src/constants.js Outdated Show resolved Hide resolved
src/constants.js Outdated Show resolved Hide resolved
src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue Outdated Show resolved Hide resolved
src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue Outdated Show resolved Hide resolved
src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue Outdated Show resolved Hide resolved
src/renderer/views/Settings/Settings.vue Outdated
Comment on lines 25 to 28
<ft-button
:label="$t('Settings.Show Keyboard Shortcuts')"
@click="showKeyboardShortcutPrompt"
/>
Copy link
Member

Choose a reason for hiding this comment

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

In your screenshot the button looks very out of place in its current position, but I can't think of anything better at the moment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair call out, I didn't know what exactly to do with this one either. This was another adjustment I tried out. Open to this one or any other suggestions. @absidue

Screenshot_20241208_085711

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it could be under general setting with a keyboard icon added to the button? Or maybe a separate keyboard shortcuts "settings section" (so it wouldnt be using a modal popup and would just display all the shortcuts under the Keyboard Shortcuts section but we'd be able to update it to support #251 eventually).

Copy link
Collaborator Author

@kommunarr kommunarr Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If it's its own setting section, I do worry that people will think it's already modifiable. I can put it on its own row there as a button for now if that works, but it might still look out of place and not fit in as a "general setting".

Edit: I do love the keyboard icon idea and incorporated that now. Still unsure on how best to improve the placement.

src/constants.js Outdated Show resolved Hide resolved
static/locales/en-US.yaml Outdated Show resolved Hide resolved
kommunarr and others added 3 commits December 8, 2024 14:37
@kommunarr @absidue
Apply suggestions from code review
1f2df14 
Co-authored-by: absidue <48293849 [email protected]>
@kommunarr
Implement review suggestions
31f93ae 
Change shortcut to be more accurate, remove duplicate action for closing current FreeTube window, update label for force reload
@kommunarr
Merge branch 'feat/keyboard-shortcut-modal' of github.com:kommunarr/F…
8476c4a 
…reeTube into feat/keyboard-shortcut-modal
@kommunarr kommunarr force-pushed the feat/keyboard-shortcut-modal branch from 5469153 to 8476c4a Compare December 8, 2024 14:53
kommunarr added a commit to kommunarr/FreeTube-Docs that referenced this pull request Dec 8, 2024
@kommunarr
Modify keyboard shortcuts to reflect changes in FreeTubeApp/FreeTube#…
d01d80f 
…6306
kommunarr added a commit to kommunarr/FreeTube-Docs that referenced this pull request Dec 8, 2024
@kommunarr
Modify keyboard shortcuts to reflect changes in FreeTubeApp/FreeTube#…
df8f4a8 
…6306
@kommunarr kommunarr mentioned this pull request Dec 8, 2024
Draft
kommunarr added a commit to kommunarr/FreeTube-Docs that referenced this pull request Dec 8, 2024
@kommunarr
Modify keyboard shortcuts to reflect changes in FreeTubeApp/FreeTube#…
8eb29cd 
…6306
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

  1. go to a video
  2. press fullwindow
  3. press shift ?
  4. See that you cant leave video player anymore

@kommunarr
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc Excellent catch, fixed and added to the testing criteria now

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Dec 9, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 9, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@PikachuEXE
Copy link
Collaborator

Center it/full width in one column view? (mobile on should just get full width regardless
image
image

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 11, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Weird suggestion:

If a contributor adds a shortcut key than they also have to :

  • open up a PR to reflect that change in the docs repo
  • add it to the keyboard shortcut modal

Would it be possible to generate the contents of the shortcut modal based on the docs? That way if someone changes something in the docs it will automatically be reflected within the app. Pull latest list on startup just like the IV instances.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Center it/full width in one column view? (mobile on should just get full width regardless

@PikachuEXE just tested is and it looks pretty good. Unsure if this is really needed

@ChunkyProgrammer
Copy link
Member

Weird suggestion:

If a contributor adds a shortcut key than they also have to :

  • open up a PR to reflect that change in the docs repo
  • add it to the keyboard shortcut modal

Would it be possible to generate the contents of the shortcut modal based on the docs? That way if someone changes something in the docs it will automatically be reflected within the app. Pull latest list on startup just like the IV instances.

I don't think we should do that. Invidious instance list is from a public api that we dont control and can be updated without a new version of FreeTube being installed. If we get the modal from the docs repo then either the modal will be incorrect for nightlies or will be incorrect for the release.

Another option is we could update the docs repo's keyboard shortcuts section to mention that you can now view the keyboard shortcuts in the app, show how to view the shortcuts and mention how that page might be out of date.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Dec 16, 2024 •
edited
Loading

Weird suggestion:

If a contributor adds a shortcut key than they also have to :

* open up a PR to reflect that change in the docs repo

* add it to the keyboard shortcut modal

Would it be possible to generate the contents of the shortcut modal based on the docs? That way if someone changes something in the docs it will automatically be reflected within the app. Pull latest list on startup just like the IV instances.

That's an interesting idea. The two main approaches I can think of would be parsing the HTML directly (which might have different formatting, preambles, etc. than we would prefer for an in-app modal) or getting it from a JSON file of shortcuts (like we do with the Invidious instances). The main issue with the latter is that we'd have to either update the JSON file as well for each change (thus being the same amount of effort of two changes per new/modified shortcut) or convert the docs page to grabbing the data dynamically from the JSON file as well, which would be shaky and might be hard to section & format. So I don't think there's a solution at the moment that would guarantee a lower amount of net effort.

Edit: Whoops, just updated to see a similar comment by @ChunkyProgrammer.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Should code comments be added to the shortcuts section so people are aware they also have to change it in the modal when they are adding/changing shortcuts?

@PikachuEXE
Copy link
Collaborator

The latest code comment doesn't seem to mention the modal

@kommunarr kommunarr force-pushed the feat/keyboard-shortcut-modal branch from 00eff2a to 31349ab Compare December 17, 2024 00:34
@kommunarr kommunarr force-pushed the feat/keyboard-shortcut-modal branch from 31349ab to 0126970 Compare December 17, 2024 00:48
@PikachuEXE
Copy link
Collaborator

Implemented #5966 but it depends on this
Reminder for myself to open PR once this merged?

@PikachuEXE PikachuEXE requested a review from absidue December 25, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

@MarmadileManteater MarmadileManteater Awaiting requested review from MarmadileManteater

@absidue absidue Awaiting requested review from absidue

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add a pop-up that displays all the keyboard shortcuts available for the player (Shift ?)
5 participants
@kommunarr @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @PikachuEXE @ChunkyProgrammer