Fix/call plugin keyBindingFns #445
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
thibaudcolas
left a comment
thibaudcolas
left a comment
There was a problem hiding this comment.
Looking pretty spot on @jacobtoppm!
I’d like to see two additions to this:
- A demo (Storybook story) of a plugin leveraging this, so we have a way to manually test this works as expected.
- An additional unit test that demonstrates #246 is indeed fixed – demoing that the editor’s built-in keyBindingFn doesn’t swallow all keyboard shortcuts.
For the demo, looking at what we have currently, you could:
- Change one of the existing ones – for example, adding a shortcut to insert embeds or section breaks.
- Add something else that might be simpler to implement – for example, showing a
window.alertwhen a keyboard shortcut is pressed. I think that would be ok enough, but ideally implementing something that’s still relevant to real-world use cases.
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
emilytoppm
force-pushed
the
fix/keybindingFn
branch
from
4ffc5af to
fa4c252
Compare
Collaborator
thibaudcolas
left a comment
thibaudcolas
left a comment
There was a problem hiding this comment.
Working really well, picked up a few small things!
| // Hack relying on the internals of Draft.js. | ||
| // See https://github.com/facebook/draft-js/pull/869 | ||
| // $FlowFixMe | ||
| const IS_MAC_OS = isOptionKeyCommand({ altKey: "test" }) === "test"; |
Collaborator
There was a problem hiding this comment.
Feel like I might want to have this be exported by Draftail, otherwise it’ll be impossible for people to display shortcuts like this without resorting to the hack. Maybe for another PR though.
thibaudcolas
approved these changes
Apr 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, as described in #246, Draftail does not call plugin
keyBindingFns, preventing custom keyboard shortcuts from being used.This PR addresses this by passing the Draftail
keyBindingFnas an extra, final plugin to the pluginEditorcomponent instead. This is done so that:keyBindingFncan continue to prevent Draft responding overly sensitively to keyboard shortcuts (otherwise we could simply use thedefaultKeyBindingsoption of the plugins editor) by returning undefinedI haven't added this to the documentation yet - as it's just a fix, I'm not sure where it should go, but happy to add if you think it needs a mention somewhere. Tested in latest Chrome and Firefox on Ubuntu.