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

Implement toggle bold and italic shortcuts #1733

Conversation

kmdupr33
Copy link

@kmdupr33 kmdupr33 commented Jan 5, 2020

Fixes #861

This pr pairs with another pr that contains changes to the gutenberg repo. It follows the proposal I laid out here.

This only contains work for the italics and bold shortcuts because the undo and redo shortcuts are already working.

  • The main listeners for shortcuts are in the GutenbergViewController and MainActivity that host the React native app. On iOS, this enables us to leverage some of the native shortcut discoverability features that are on iOS as well as making shortcuts "global" for free. If we agree this is a reasonable choice on iOS, I'll need to add localized strings for shortcut discoverability (see checkbox below). For consistency and for making it easy to make shortcuts global, I also implemented the shortcut listener in the MainActivity on android.
  • On iOS, since the GutenbergViewController is already pretty long, I've created a ShortcutsCoordinator in the same spirit as the Media*Coordinators. In order for the shortcuts coordinator to do non-trivial work in handling shortcuts, I needed to modifier the responder chain so that it has a chance to handle shortcut events. On Android, the ideal home for this functionality was less obvious, and since the MainActivity is pretty lean, I threw some of the shortcut code there for now. I'm totally open to feedback if anyone has any ideas where that code should live.
  • A key assumption I made here is that keyboard shortcuts for us are always global and don't have target elements, which is unlike web's implementation. I figured this was kosher, but if I'm wrong about this, I'll need to rethink some things on the js side and on the native side.

To test:

If you want to test the bold shortcut, its as simple as clicking a textview and pressing modifier-b.

If you want to test italics, you'll have to run the apps in release mode so that the developer menu is disabled and the toggle inspector shortcut doesn't interfere with the toggle italics shortcut.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@kmdupr33 kmdupr33 force-pushed the issue-861-keyboard-shortcuts branch 2 times, most recently from 03ea4ef to 30c7088 Compare January 5, 2020 04:21
@kmdupr33 kmdupr33 changed the title wip Implement toggle bold and italic shortcuts Jan 6, 2020
@kmdupr33 kmdupr33 force-pushed the issue-861-keyboard-shortcuts branch 2 times, most recently from 718780f to 0d5199d Compare January 7, 2020 03:43
@kmdupr33 kmdupr33 marked this pull request as ready for review January 7, 2020 03:44
@kmdupr33 kmdupr33 force-pushed the issue-861-keyboard-shortcuts branch 8 times, most recently from ae276bb to 15e6e37 Compare January 12, 2020 15:26
@kmdupr33 kmdupr33 force-pushed the issue-861-keyboard-shortcuts branch from 15e6e37 to 2ed7a7e Compare January 14, 2020 15:05
@hypest
Copy link
Contributor

hypest commented Jun 12, 2020

Closing this as not updated in a while.

@hypest hypest closed this Jun 12, 2020
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.

Keyboard shortcuts
3 participants