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

Keyboard shortcuts for post editor: bold, italic, link #11189

Closed

Conversation

ludwiczakpawel
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

It was driving me nuts I couldn't use keyboard shortcuts for post editor so I copy'pasted some code and brought the shortcuts to the editor... It kinds works sooo πŸ€·β€β™‚οΈ :D

I'm happy to replace it with "more professional" solution though since I'm a JS n00b. And this solution may be completely wrong, I don't know...

QA Instructions, Screenshots, Recordings

Go to /new, write some text and try CMD+I, CMD+B and CMD+K.

Added tests?

  • Yes
  • No, not needed.
  • I need help with writing tests

Added to documentation?

  • Docs.forem.com
  • README
  • No documentation needed

@ludwiczakpawel ludwiczakpawel added the area: publishing experience issues related to an authors experience publishing. Tags, series, etc. label Oct 30, 2020
@ludwiczakpawel ludwiczakpawel self-assigned this Oct 30, 2020
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 30, 2020
@Link2Twenty
Copy link
Contributor

Link2Twenty commented Oct 30, 2020

We have a keyboard shortcut hook already so it might be better to use that

#10713

You'd probably wanna do something like this

import { useKeyboardShortcuts } from '../../shared/components/useKeyboardShortcuts';
import { handleBoldAndItalic, handleLink  } from '@utilities/editorKeyboardShortcuts';

useKeyboardShortcuts({
'ctrl+KeyI': handleBoldAndItalic, 
'ctrl+KeyB': handleBoldAndItalic,
'ctrl+KeyK': handleLink,
}, textAreaRef);

Then export your helper functions from editorKeyboardShortcuts

@ludwiczakpawel
Copy link
Contributor Author

ludwiczakpawel commented Oct 30, 2020

@Link2Twenty oh sweeet! :) as I said, I'm a JS moron but I'll try :))) Thanks!

(taking that into consideration, I'm turning this PR into Draft mode)

@ludwiczakpawel ludwiczakpawel marked this pull request as draft October 30, 2020 16:44
@Link2Twenty
Copy link
Contributor

@Link2Twenty oh sweeet! :) as I said, I'm a JS moron but I'll try :))) Thanks!

(taking that into consideration, I'm turning this PR into Draft mode)

Would it be helpful if I went through an commented where my bits of code go in, or have you got it? 😊

@ludwiczakpawel
Copy link
Contributor Author

@Link2Twenty absolutely yes!

In fact, we could refactor it in two places if possible because we also have these functions on initializeCommentsPage.js.erb too

@@ -0,0 +1,76 @@
const KEY_CODE_B = 66;
const KEY_CODE_I = 73;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can go

@pr-triage pr-triage bot added PR: draft bot applied label for PR's that are a work in progress and removed PR: unreviewed bot applied label for PR's with no review labels Oct 30, 2020
@@ -0,0 +1,76 @@
const KEY_CODE_B = 66;
const KEY_CODE_I = 73;
const KEY_CODE_K = 75;
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can go

const KEY_CODE_I = 73;
const KEY_CODE_K = 75;

export function handleKeyDown(e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function can go

}
}

function handleBoldAndItalic(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function will be exported

);
}

function handleLink(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function will be exported

@@ -10,6 +10,7 @@ import {
onDragExit,
} from './dragAndDropHelpers';
import { useDragAndDrop } from '@utilities/dragAndDrop';
import { handleKeyDown } from '@utilities/editorKeyboardShortcuts';
Copy link
Contributor

Choose a reason for hiding this comment

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

This line becomes

import { handleBoldAndItalic, handleLink } from '@utilities/editorKeyboardShortcuts';

@@ -70,6 +71,9 @@ export const EditorBody = ({
placeholder="Write your post content here..."
value={defaultValue}
onInput={onChange}
onKeyDown={(e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This prop can go

@@ -10,6 +10,7 @@ import {
onDragExit,
} from './dragAndDropHelpers';
import { useDragAndDrop } from '@utilities/dragAndDrop';
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to add an new import in here somewhere

import { useKeyboardShortcuts } from '../../shared/components/useKeyboardShortcuts';

@@ -10,6 +10,7 @@ import {
onDragExit,
} from './dragAndDropHelpers';
import { useDragAndDrop } from '@utilities/dragAndDrop';
import { handleKeyDown } from '@utilities/editorKeyboardShortcuts';

function handleImageSuccess(textAreaRef) {
return function (response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Around line 45 we want to add

useKeyboardShortcuts({
'ctrl+KeyI': handleBoldAndItalic, 
'ctrl+KeyB': handleBoldAndItalic,
'ctrl+KeyK': handleLink,
}, textAreaRef);

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been testing and

  useEffect(() => {
    const { current } = textAreaRef;
    if (!current) return;
    useKeyboardShortcuts({
      'ctrl+KeyI': handleItalic,
      'ctrl+KeyB': handleBold,
      'ctrl+KeyK': handleLink,
    }, current.base);
  }, [textAreaRef]);

Just under

  useEffect(() => {
    if (textAreaRef.current) {
      setElement(textAreaRef.current.base);
    }
  });

works

@Link2Twenty
Copy link
Contributor

You may also want to move app/javascript/utilities/editorKeyboardShortcuts.js to app/javascript/article-form/components/keyboardShortcutHelpers.js to be more inline with drag and drop


function replaceSelectedText(textArea, text) {
// Chrome and other modern browsers (except FF and IE 8,9,10,11)
if (document.execCommand('insertText', false, text)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you missed something here

@Link2Twenty
Copy link
Contributor

dev-no-toggle

We'll need to consider how to fix toggling too.

@Link2Twenty
Copy link
Contributor

Link2Twenty commented Oct 30, 2020

So execCommand is obsolete meaning we probably shouldn't use it. setRangeText is fine to use but does not put changes into the undo pool. I can continue to look into a solution. Here is modification to your helper functions to allow toggling and to not use execCommand.

export function handleBold(event) {
  event.preventDefault();

  _toggleSurroundText(event.target, '**');
}

export function handleItalic(event) {
  event.preventDefault();

  _toggleSurroundText(event.target, '__');
}

export function handleLink(event) {
  event.preventDefault();
  const textArea = event.target;

  const selection = textArea.value.substring(
    textArea.selectionStart,
    textArea.selectionEnd,
  );
  const selectionStart = textArea.selectionStart;
  const selectionEnd = textArea.selectionEnd;

  textArea.setRangeText(`[${selection}](url)`, selectionStart, selectionEnd);

  // start position + length of selection + [](
  const startOffset = selectionStart + selection.length + 3;

  // start offset + 'url'.length
  const endOffset = startOffset + 3;

  textArea.setSelectionRange(startOffset, endOffset);
}

function _toggleSurroundText(textArea, surroundingStr) {
  // text to modify
  const selection = textArea.value.substring(
    textArea.selectionStart,
    textArea.selectionEnd,
  );

  // surrounding area for checks
  const expandedSelection = textArea.value.substring(
    textArea.selectionStart - surroundingStr.length,
    textArea.selectionEnd + surroundingStr.length,
  );

  // current selection position
  let selectionStart = textArea.selectionStart;
  let selectionEnd = selectionStart + selection.length

  // position and string once transformed
  let newString = `${surroundingStr}${selection}${surroundingStr}`;
  let newStart = selectionStart + surroundingStr.length;
  let newEnd = selectionEnd + surroundingStr.length;

  // if it's already been set to bold/italics toggle back.
  if (expandedSelection === newString) {
    // selection position to include existing tag
    selectionStart = selectionStart - surroundingStr.length;
    selectionEnd = selectionEnd + surroundingStr.length;

    // position and string once transformed
    newString = selection;
    newStart = selectionStart;
    newEnd = newStart + selection.length;
  }

  textArea.setRangeText(newString, selectionStart, selectionEnd);
  textArea.setSelectionRange(newStart, newEnd);
}

after a little bit of research people either use the obsolete execCommand (which I wouldn't recommend) or handle their own history (catching ctrl+z and ctrl+y to handle this).

We could of course use an open source markdown editor like https://simplemde.com/ that will handle everything for us. Or even make and maintain our own.

@citizen428
Copy link
Contributor

Hi @ludwiczakpawel, were you thinking about picking this up again? If not, maybe @Link2Twenty would be nice enough to address this in a new PR?

@ludwiczakpawel
Copy link
Contributor Author

@citizen428 I'd happy to hand it off.

@thomasbnt
Copy link
Contributor

Oooh yay, interesting topic πŸ‘€

@citizen428
Copy link
Contributor

@thomasbnt Would you be interested in taking this over and finishing it? πŸ˜ƒ

@Link2Twenty
Copy link
Contributor

Someone over on forem.dev mentioned they'd made an external rich text editor that will produce forem valid markdown.

I think moving over to real rich text would solve this issue but also make a better editing experience in general. It's too big a project for me to commit to though πŸ˜…

Links:

@thomasbnt
Copy link
Contributor

thomasbnt commented Apr 19, 2021

@thomasbnt Would you be interested in taking this over and finishing it?

Hi @citizen428 ! I'm in procedure install (On Arch) for the first time, and I don't know if I'm going to get it all right. Maybe I could take this PR later, but it's not sure so if someone can do it, especially since it looks pretty complicated

Edit :
I can't do the installation procedure, I already have another version of NodeJS that I use daily, so for the moment I'm leaving out the fact of helping... it bothers me a bit

@nickytonline nickytonline requested review from aitchiss and removed request for benhalpern and a team April 20, 2021 04:18
@citizen428
Copy link
Contributor

@Link2Twenty Since you commented here previously, any chance you want to take over this PR and finish it? πŸ˜ƒ No worries if you don't have time for that, I just thought I'd ask.

@Link2Twenty
Copy link
Contributor

@citizen428 with the version 3 editor (Rich Text/WYSIWYG) on the roadmap for this year is it worth waiting for that? I would presume that would have all the keyboard shortcuts built in.

@citizen428
Copy link
Contributor

Good point @Link2Twenty. I guess I was mostly looking for something quick that can bridge that gap (i.e. just incorporating some of the changes you suggested here) but I understand if that's not super appealing.

@Link2Twenty
Copy link
Contributor

Link2Twenty commented May 4, 2021

I just discovered something interesting.

I made a note over on #13234 about how we could use contenteditable divs rather than textareas, here's the demo again, because they auto size change. What I didn't realise at the time was they also take ctrl+b and ctrl+i by default.

cc @aitchiss


EDIT: only in chrome it seems πŸ˜…

@aitchiss
Copy link
Contributor

Hey @Link2Twenty I don't think we want to move our text areas over to contenteditable for this particular PR. As the editor and the backend rely on markdown, we need the text content of the area to change to e.g. **bold** on ctrl + b, so even in chrome where this looks like it's working, it doesn't meet our needs unfortunately πŸ™

I think the shortcuts in this PR would still be useful, like @citizen428 mentioned, bridging the gap until we have more functionality in the editor. If you'd like to take it on, let us know, but of course no worries if not!

@citizen428
Copy link
Contributor

Hi @Link2Twenty πŸ‘‹ Do you think you'll work on this PR? If not it's probably time to close it for now and revisit this topic later.

@Link2Twenty
Copy link
Contributor

I don't think I'll have time to work on this I'm afraid.

@citizen428
Copy link
Contributor

Thanks for the quick reply @Link2Twenty!

@citizen428 citizen428 closed this Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: publishing experience issues related to an authors experience publishing. Tags, series, etc. PR: draft bot applied label for PR's that are a work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants