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

Desktop: Add keyboard shortcuts for inserting lists #5137

Merged
merged 1 commit into from
Jul 9, 2021

Conversation

Philipp91
Copy link
Contributor

@Philipp91 Philipp91 commented Jun 30, 2021

See also the feature request at tinymce/tinymce#6700. I later realized that Joplin has its own lists.js plugin, so I implemented it there.

Tested manually on Linux (KDE), it works.

Open questions:

  • Should I reflect these changes in Assets/TinyMCE/JoplinLists/src/main/ts/...?
  • (Obsolete) Should I move the new lines to line 2055 instead (under the addCommand() calls)? There, they would be active even when hasPlugin(editor, 'advlist') -- not sure what that is and so I'm not sure which one makes sense. I.e. only add the keyboard shortcut if the button exists, or always?
  • Should these keyboard shortcuts be advertised somewhere (same goes for bold, insert link, etc.), e.g. in the button tooltips?
  • Should I add the same shortcuts for the Markdown editor (maybe someone can give a code pointer)?

@laurent22
Copy link
Owner

I guess the list plugin somehow exposes these commands to the editor? In that case, isn't it possible to assign these shortcut at the editor level? Because I'd rather no hard-code shortcuts in the list as it means it would be difficult to customise. I don't mind if it's hard-coded on the editor side as we can always improve that later on.

@Philipp91
Copy link
Contributor Author

Okay, I've moved them to TinyMCE.tsx now, so they're out of the plugin. All the other shortcuts (like Ctrl+B) are hard-coded in the tinymce code base (i.e. inside node_modules). Just like this newly added shortcuts, they can't be changed from the Joplin settings either.

@laurent22
Copy link
Owner

Ok thanks for the update. There are several white space changes - please could you remove them?

See also the feature request at tinymce/tinymce#6700.

Tested manually, it works.
@Philipp91 Philipp91 marked this pull request as ready for review July 6, 2021 19:59
@Philipp91
Copy link
Contributor Author

There are several white space changes - please could you remove them?

Done. What about the open questions in the OP?

@laurent22
Copy link
Owner

Looks good now, thanks for the update. As for your other questions:

Should these keyboard shortcuts be advertised somewhere (same goes for bold, insert link, etc.), e.g. in the button tooltips?

Normally we create a command, which is then added to the menu, so that users can find out what command is triggered by what shortcut. I don't know how easy or hard it would be to add these commands to the menu.

Should I add the same shortcuts for the Markdown editor (maybe someone can give a code pointer)?

Yes I'd have accept a pull request for this, but it doesn't have to be in this pull request.

@laurent22 laurent22 merged commit bc97bb2 into laurent22:dev Jul 9, 2021
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