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

Split shortcuts by platform and name them #591

Closed
wants to merge 1 commit into from

Conversation

hpohlmeyer
Copy link

The main task of this pull request is to name the default shortcuts.

I think the default shortcuts are not bad, but if you want to use mobiledoc kit as a framework you need a way to reset the shortcuts and not just overwrite them. (For example, if you do not want to support underlined text)

You already have the unregisterKeyCommands() function to delete key commands. Unfortunately, the shortcuts need a name property to delete them. This is what I did for the default commands.

Some of them had different functions, depending on the platform. This is why I split the arrays into a mac and a windows array. This way I could give them distinctive names.

@hpohlmeyer
Copy link
Author

Another approach to this would be to change the unregisterKeyCommands() function to take the key command instead of a name. This way you do not even remember the name of the shortcut, but can delete the unwanted shortcut.

@hpohlmeyer
Copy link
Author

Travis CI fails, because some of the key commands don’t work anymore. For example CTRL+B worked on macs, but did the same as CMD+B. All windows commands, that use CTRL as a modifier are now associated with CMD on mac.

Before I start modifing the test, I would like to know if you welcome these changes. Thanks!

@mixonic
Copy link
Contributor

mixonic commented Nov 28, 2017

@hpohlmeyer thanks for diving in here, I've made a note to try and follow up on Friday. 🙇

@lukemelia
Copy link
Contributor

I ran into a similar issue as @hpohlmeyer and am supportive of his proposed change.

@hpohlmeyer
Copy link
Author

Closing this as @lukemelia has a modernized version of this PR in #789.

@hpohlmeyer hpohlmeyer closed this Sep 18, 2023
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.

3 participants