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

Feature: Display keyboard shortcuts in the right click menu #13120

Merged

Conversation

ferrariofilippo
Copy link
Contributor

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Feature: Display keyboard shortcuts in the right click menu #13055

When there's more than one shortcut, should the label display all of them?
There's a bug involving Enter and I'm working on it (if you hit Enter, which is the accelerator for Open, it will cut the item, since that's the first button in the menu)

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?
    1. Open app
    2. Right click on a file / empty space

Screenshots (optional)
image

@ferrariofilippo
Copy link
Contributor Author

There's a bug involving Enter and I'm working on it (if you hit Enter, which is the accelerator for Open, it will cut the item, since that's the first button in the menu)

@yaira2 I thought that if we allow Enter accelerator in the context flyout, users won't be able to work with it using only the keyboard. You need to hit Enter to select an action, but doing so would trigger the shortcut.
I think we should prevent this. We may remap Enter shortcuts to something else (like Ctrl + Enter or Shift + Enter) only in the context menu

@yaira2
Copy link
Member

yaira2 commented Aug 3, 2023

@ferrariofilippo is it possible to display the shortcut without adding the accelerators to the context menu? It might be confusing if we used custom shortcuts only in the context menu.

@ferrariofilippo
Copy link
Contributor Author

@ferrariofilippo is it possible to display the shortcut without adding the accelerators to the context menu? It might be confusing if we used custom shortcuts only in the context menu.

The label would be visible, but the shortcut would not work.
I don't want to use custom shortcuts in the context menu. I think we should change only Enter shortcut to something else, since it would make the context menu inaccessible to keyboard users (all the other shortcuts won't be affected [Ctrl + C, Ctrl + V, Ctrl + Enter, ...]).

@yaira2
Copy link
Member

yaira2 commented Aug 3, 2023

The label would be visible, but the shortcut would not work.

Wouldn't the shortcut work from the rich commands?

@yaira2
Copy link
Member

yaira2 commented Aug 3, 2023

Regarding enter, we can leave that blank 👍

@ferrariofilippo
Copy link
Contributor Author

The label would be visible, but the shortcut would not work.

Wouldn't the shortcut work from the rich commands?

It seems the context menu handles all the inputs while open, the RichCommands are not invoked

@yaira2
Copy link
Member

yaira2 commented Aug 3, 2023

That's good to know, in that case we can just leave enter blank and fill out the other ones.

@ferrariofilippo
Copy link
Contributor Author

What about this?
image

Should we show all the available shortcuts or only the first one?

@yaira2
Copy link
Member

yaira2 commented Aug 3, 2023

Just the first one

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 merged commit 92f8ee0 into files-community:main Aug 3, 2023
1 check passed
@ferrariofilippo ferrariofilippo deleted the Keyboard_Accelerator_Label branch August 3, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Display keyboard shortcuts in the right click menu
2 participants