Mobile: Add keyboard-activatable markdown commands (e.g. bold, italicize)

[personalizedrefrigerator]

Summary

• Creates a set of commands that can be used by a future markdown toolbar.
• Adds a find/replace dialog and a 'edit link' dialog that can be opened with ctrl+F/ctrl+K.
  • 
  • 
• Connects some of these commands to keyboard shortcuts (e.g. ctrl+F opens a ReactNative component for find/replace, ctrl+` toggles code, ctrl+B bolds).

Keyboard shortcuts that are present in Joplin desktop, but not in this PR:

• ctrl+/ = comment

There may be others...

Keyboard shortcuts not present in Joplin desktop, but that users might expect

• ctrl+H = toggle find/replace
• alt+1 = toggle heading level 1
• alt+2 = toggle heading level 2
• ...
• ctrl+shift+. = add quote

I haven't implemented these shortcuts in this PR. alt+1, alt+2, ..., alt+6 are very easy to implement. Should I add them to this PR?

[laurent22]

Looks good overall. For the React Hooks, please make sure you always pass all the dependencies to the effects (there's an eslint rule to detect this normally but unfortunately we can't currently use it).

There are still tests with nested describes - my concern is, when a test fails, how easy is it to run just the failing test in these nested configurations? Could you try and confirm please? Fail one of the test, then see what command is required to run just that test (based only on the jest output). If you could post the result here that will allow us to make a decision about these nested describes.

[laurent22]

Please check how other tests are written. The describe part should have the same name as the filename (minus .test.ts). Again this is to make it easier to run a specific tests or group of tests.

[personalizedrefrigerator]

Fixed!

[laurent22]

That's brilliant you can create the editor inside the test and run commands directly. Going to make things so much more reliable.

[laurent22]

Please make sure to add all dependencies to the array.

But it should strictly be what's required and nothing more - so editorLinkData.linkText, props.selectionState.selectedText, editorLinkData.linkURL (and not editorLinkData, props.selectionState)

[laurent22]

Please declare all ref, state, etc at the the top of the hook, because they can be used anywhere within the function.

[laurent22]

All function that are used in callbacks should be defined using useCallback to avoid stale data, and used variables should be passed in the dependencies array. Also please name it onSomething (here onSubmit) to make it clear it's a callback. Maybe we need some eslint rule for this.

[laurent22]

This should have the Theme type (unless it's not possible to import it here for some reason).

[personalizedrefrigerator]

I wasn't aware that type existed! Good catch!

[laurent22]

Again please don't use this pattern.

• If an object interface is used more than once, define a properly named interface

• If it's used only once, there's a chance you don't need a interface. In this particular case, just use regular function arguments. That way you don't need to repeat the names twice over two lines.

[personalizedrefrigerator]

Good catch! (I wrote this code a while ago and forgot to update it.)

[personalizedrefrigerator]

Fail one of the test, then see what command is required to run just that test (based only on the jest output).

Suppose we have the following output:

 FAIL  components/NoteEditor/CodeMirror/markdownReformatter.test.ts (18.674 s)
  ● markdownReformatter › should match start and end of bolded regions › should return -1 if no match is found

    expect(received).toBe(expected) // Object.is equality

    Expected: 1
    Received: 0

      28 |                      const doc = DocumentText.of(['**...** test.']);
      29 |                      const sel = EditorSelection.range(3, 3);
    > 30 |                      expect(0).toBe(1);
         |                                ^
      31 |                      expect(findInlineMatch(doc, spec, sel, MatchSide.Start)).toBe(-1);
      32 |              });
      33 |      });

      at Object.<anonymous> (components/NoteEditor/CodeMirror/markdownReformatter.test.ts:30:14)

Running

yarn test --testNamePattern 'should match start and end of bolded regions should return -1 if no match is found' --runTestsByPath components/NoteEditor/CodeMirror/markdownReformatter.test.ts

runs the failing test.

Of course,

yarn test -f

or

yarn test --onlyFailures

also work, but they require knowledge of which tests failed in the last execution.

[laurent22]

Thanks for checking for the tests, but I feel that the extra complexity of nested describes is not worth it. We managed without this for several years, so hopefully we can keep doing so. So please could you remove these nested describes and replace them with a flat structure?

[laurent22]

I think it's good now, thanks for adding this!