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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change keyboard shortcut to access + z #10008

Merged
merged 1 commit into from Sep 21, 2018

Conversation

Projects
None yet
4 participants
@talldan
Contributor

talldan commented Sep 18, 2018

Description

Fixes #9940
Fixes #9036

Second attempt at this after #9190 had to be reverted.

Changes the shortcut for remove block to access + z (ctrl + option + z on a Mac / shift + alt + z elsewhere).

Changes:

  • change the shortcut in the editor-global-shortcuts
  • change the shortcut in the keyboard shortcuts modal
  • change the shortcut in the docs
  • change the shortcut in the e2e tests
  • remove some now unused code in RichText.

This requires some cross browser testing. Please help if you can 馃槃

How has this been tested?

Screenshots

screen shot 2018-09-18 at 3 50 45 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 18, 2018

Contributor

Works fine for me on Antergos Linux using Firefox Beta and Chromium. 馃憤

Contributor

ZebulanStanphill commented Sep 18, 2018

Works fine for me on Antergos Linux using Firefox Beta and Chromium. 馃憤

@youknowriad

Confirmed it works and fixes the bug removing two blocks instead of one.

@jasmussen Thoughts on the chosen shortcut?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 21, 2018

Contributor

I'm sort of fine with us picking a keyboard shortcut at this point just to get things going.

Z does feel very very tied to "undo" though, so I'm not loving it. But I also don't have better suggestions if we can't use del or backspace keys.


On a separate note, but posting here because it's related to keyboard shortcuts, we've had long discussions about using keys that are physically located on a specific place on the keyboard. Like the key below escape, which happens to be a backtick on US keyboards but it's $ on a Danish one.

Could we use https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code to simplify this?

Contributor

jasmussen commented Sep 21, 2018

I'm sort of fine with us picking a keyboard shortcut at this point just to get things going.

Z does feel very very tied to "undo" though, so I'm not loving it. But I also don't have better suggestions if we can't use del or backspace keys.


On a separate note, but posting here because it's related to keyboard shortcuts, we've had long discussions about using keys that are physically located on a specific place on the keyboard. Like the key below escape, which happens to be a backtick on US keyboards but it's $ on a Danish one.

Could we use https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/code to simplify this?

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 21, 2018

Contributor

Thanks for the review feedback! I've tested IE and Edge, and they work fine.

@jasmussen code returns the character associated with the physical key, so I'm not sure it helps in your example. A Danish keyboard would return something like code: Tilde while a US one code: Backquote. If I remap my keyboard it also won't take that into account, still returning the physical key.

It still also results in the issue where some keyboards might not have a particular key at all, rendering the shortcut unusable.

It's frustrating how difficult it is and I don't think there's a way to have one solution that fits all. When I've looked into how others have achieved it, they've often had to 'cheat' by mapping a single action to multiple keys and in some cases they also don't work very well.

We'd probably be better off making keyboard shortcuts remappable within Gutenberg, and maybe enabling different profiles that can be switched between (these could even be plugins).

Contributor

talldan commented Sep 21, 2018

Thanks for the review feedback! I've tested IE and Edge, and they work fine.

@jasmussen code returns the character associated with the physical key, so I'm not sure it helps in your example. A Danish keyboard would return something like code: Tilde while a US one code: Backquote. If I remap my keyboard it also won't take that into account, still returning the physical key.

It still also results in the issue where some keyboards might not have a particular key at all, rendering the shortcut unusable.

It's frustrating how difficult it is and I don't think there's a way to have one solution that fits all. When I've looked into how others have achieved it, they've often had to 'cheat' by mapping a single action to multiple keys and in some cases they also don't work very well.

We'd probably be better off making keyboard shortcuts remappable within Gutenberg, and maybe enabling different profiles that can be switched between (these could even be plugins).

@talldan talldan merged commit 2ff11bd into master Sep 21, 2018

2 checks passed

codecov/project 48.81% (+<.01%) compared to 779c3c5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talldan talldan deleted the fix/delete-block-shortcut branch Sep 21, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 21, 2018

Contributor

Ah thanks for the answer. I read something about "scancodes" recently, and supposedly that could help us achieve what we wanted, but seems like there's no JS counterpart.

Contributor

jasmussen commented Sep 21, 2018

Ah thanks for the answer. I read something about "scancodes" recently, and supposedly that could help us achieve what we wanted, but seems like there's no JS counterpart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment