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

Add mousetrap bindings for bold, italics, and links in TextEditor #2725

Merged
merged 2 commits into from
Jul 9, 2020

Conversation

todtb
Copy link
Contributor

@todtb todtb commented Jul 9, 2020

#2722

This adds mousetrap bindings for bold, italics, and links in the TextEditor component. The use of refs required converting TextEditorFormatButton from a functional component.

Additionally, this can't really be handled in the keyboard_shortcuts.js file as we need to bind to an element rendered by the component rather than the default document.

I'm not 100% certain this is the cleanest approach, but it seems to click-test well. Open to any suggestions to make it more idiomatic.

Copy link
Member

@kueda kueda left a comment

Choose a reason for hiding this comment

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

Honestly I think this is pretty much the ideal use case for refs and class-base components, so I don't think that's a problem. Personally I like your more modular approach to defining the shortcuts. Definitely makes this easier to re-use.

Aside from the style issues I've noted (in this case we do have linter rules), the only hiccup I'm noticing is that in Firefox 78.0.2, CMD-b and CMD-i do other things in the browser (open the bookmarks sidebar and the Page Info popup, respectively), both of which cause the textarea to lose focus. CTRL-b seems to move the cursor back, and CTRL-i does nothing. Github's text editor doesn't seem to have the weird CMD side effects in Firefox, so maybe there's a way to suppress that? A workaround might be to be specific about supporting both CMD- and CTRL- shortcuts instead of using mod-. Not sure we need to address this issue now, but it would certainly be nice if you can find a solution.

app/webpack/shared/components/text_editor.jsx Outdated Show resolved Hide resolved
app/webpack/shared/components/text_editor.jsx Outdated Show resolved Hide resolved
@todtb
Copy link
Contributor Author

todtb commented Jul 9, 2020

I tested preventDefualt( ) on 78.0.2 (64-bit) and that should do the trick. I think that's acceptable in this case given that it is only bound to the textarea.

Not sure if we need to support IE, but it (of course) uses a slightly different approach I've left out for now.

@kueda kueda merged commit 95e0556 into inaturalist:master Jul 9, 2020
@kueda
Copy link
Member

kueda commented Jul 9, 2020

Looking great. Thanks again!

@todtb todtb deleted the 2722-texteditor-keyboard-shortcuts branch July 10, 2020 01:07
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.

2 participants