• 
      

    Markdown formatting toolbar for rich text fields.

    Review Request #11004 — Created May 15, 2020 and submitted

    Information

    Review Board
    master
    f1fc4f0...

    Reviewers

    This patch adds a markdown formatting toolbar for rich text editors.
    The buttons for inline formatting add the corresponding syntax to the
    selection if one exists, or just inserts the syntax and positions
    the cursor. This applies to the bold, italic, strikethrough, ordered
    and unordered list, and code buttons. If the selection begins and ends
    with the same syntax as the button being pressed, the syntax is removed.
    For example, if the bold button is pressed and the selection is already
    bolded, the bold syntax is removed.

    For the insert link button, if there is a selection, the selection
    becomes the text to display for the url. Otherwise, it inserts syntax
    for a link and positions the cursor where the text to display will go.
    Positioning the cursor in a formatted link, selecting the text in the
    link, or the either link will remove the formatting.

    For the upload image button, it opens a dialog to upload an image.
    A reference to the image is added to the editor.

    For the ordered and unordered list buttons, numbers (1.) or dashes (-)
    are appended to or removed from to the beginning of the current line.

    Unit tests for each type of formatting with:
    - an empty selection
    - cursor positioned in text
    - unformatted text selection
    - formatted text selection with text only, not including syntax
    - formatted text selection including both text & syntax


    Description From Last Updated

    I think it'd look nice to have this show up within the editor. Kind of like in Asana. By having …

    chipx86chipx86

    Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _)

    reviewbotreviewbot

    This bit looks a bit weird; it's for the upload image button, which is different from the other buttons because …

    hxqlinhxqlin

    The button that toggles the rich text attribute is a bit tangled up in inlineEditorView so I wasn't sure how …

    hxqlinhxqlin

    Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _)

    reviewbotreviewbot

    Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _)

    reviewbotreviewbot

    Since we're in the final stages of this, it'd be nice to move this into rb/css/ui/formatting-toolbar.less and to use our …

    chipx86chipx86

    It's best not to override the function. Instead, let's bind it when we pass it to the constructor below.

    chipx86chipx86

    To stay consistent, the pattern we normally use is: this._$toolbar.$el.appendTo(this.$el);

    chipx86chipx86

    Two blank lines between top-level classes or functions.

    chipx86chipx86

    Option arguments should be documented in an Option Args section, following Args. These work just like other arguments, listing names, …

    chipx86chipx86

    Can you have this wrap? You can separate it into a chain of jQuery statements.

    chipx86chipx86

    Element building should happen in render().

    chipx86chipx86

    This can be one line, since we're not chaining.

    chipx86chipx86

    We currently avoid for .. in, due to Babel's expensive, verbose transpilation. Can you convert this to a standard for …

    chipx86chipx86

    Missing quotes around ${id}, to make the tag valid.

    chipx86chipx86

    The .addClass should be indented 4 spaces.

    chipx86chipx86

    I think this can be consolidated a bit, and updated to be more explicit at the same time. How about: …

    chipx86chipx86

    Let's name buttonInfo options instead, and add an Option Args section so it's clear what's allowed by the caller. Also …

    chipx86chipx86

    We're largely moving to .on() instead of shortcuts like .click() for clarity (and to avoid some issues with edge cases …

    chipx86chipx86

    For a setter, "Set the position of the cursor" would be more correct. This also isn't specific to emphasis.

    chipx86chipx86

    We fetch this._codeMirror multiple times. Let's pull it out into a variable to reduce lookups.

    chipx86chipx86

    "Toggle"

    chipx86chipx86

    A few notes: You'll need to escape these. You should use double backticks around these, instead of quotes. The backtick …

    chipx86chipx86

    I don't think the term "emphasis" is quite right. In Markdown, it can refer to bold/italic, but not strikethrough or …

    chipx86chipx86

    Let's pull this._codeMirror out into a local variable here as well. We access it a lot.

    chipx86chipx86

    The * should all align.

    chipx86chipx86

    We don't use [..., ...] = ... syntax in our codebase, because Babel transforms that into some lengthy code. Let's …

    chipx86chipx86

    I want to dive into this regex with you, because I'm not confident it'll do what you want. Let's take …

    chipx86chipx86

    The asterisks should align.

    chipx86chipx86

    Formatting isn't quite right here. This should look like: const newSelection = selection.replace( new RegExp(...), '');

    chipx86chipx86

    More cases of this regex. I count 3. Once we figure out what it should be, we should have a …

    chipx86chipx86

    Asterisks should align.

    chipx86chipx86

    Seeing inconsistency on indentation. Can you make sure this is correct all throughout?

    chipx86chipx86

    Same note as above with the [..., ...] = ... syntax.

    chipx86chipx86

    THis can be a single-line comment.

    chipx86chipx86

    This can be a singline line comment.

    chipx86chipx86

    This can be a single line comment.

    chipx86chipx86

    This line is too long. Can you wrap the second parameter to the next line>?

    chipx86chipx86

    No blank line here.

    chipx86chipx86

    This needs a "Returns" section.

    chipx86chipx86

    "Find"

    chipx86chipx86

    We access this._codeMirror many times. Let's pull it out into a variable.

    chipx86chipx86

    Let's make this: for (let curToken = codeMirror.getTokenAt(cursorStart, true); curToken.string !== ' ' && groupStart.ch !== 0; curToken = codeMirror.getTokenAt(groupStart, …

    chipx86chipx86

    You can use -= here.

    chipx86chipx86

    You can use -=.

    chipx86chipx86

    We can pull this._codeMirror out into a local variable.

    chipx86chipx86

    "Insert"

    chipx86chipx86

    "Markdown"

    chipx86chipx86

    Let's pull this._codeMirror out into a local variable.

    chipx86chipx86

    All arguments to a function should align.

    chipx86chipx86

    This line is too long. Also, we prefer to keep logic out of the ${...} references in format strings. Let's …

    chipx86chipx86

    These lines are too long. Let's wrap: const precedingText = codeMirror.getLineTokens(...) .filter(...) ...

    chipx86chipx86

    This line is too long. Let's do: codeMirror.setSelection({ ch: ..., line: ..., }); And use alphabetical order on the keys.

    chipx86chipx86

    Same notes as above with the expression and line lengths.

    chipx86chipx86

    We avoid for ... of syntax due to Babel transpiling this into an expensive block of code. Let's just use …

    chipx86chipx86

    Col: 23 Invalid regular expression.

    reviewbotreviewbot
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    hxqlin
    1. This doesn't quite have everything from the project description just yet. I'm not sure if the current implementation is easily open to modification by extensions though so I thought I'd just put up an initial review request to see if there's something big.

    2. Show all issues

      This bit looks a bit weird; it's for the upload image button, which is different from the other buttons because it's an input hidden in a label. It needs the _uploadImage function from RB.TextEditorView but also needs access to the input element to get the file. There must be a nicer way to do it but I'm not sure how.

      1. How about registering .rb-c-formatting-toolbar__image-btn change in events: {...} to _onFileUploadChanged, and having the event handler logic there? That'd keep this code generic.

    3. 
        
    hxqlin
    Review request changed
    Change Summary:

    Change the markdown checkbox to a button that toggles markdown in the field when clicked.

    Commit:
    8d2d3792ceaaaae09015e69fd4101ae426c8ad84
    057181aa978c1126ce8f7a75a71f0997ce0825b5

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    hxqlin
    1. 
        
    2. Show all issues

      The button that toggles the rich text attribute is a bit tangled up in inlineEditorView so I wasn't sure how to pull it out to display on the right hand side of the text field where I think it'd also look better in line with the rest of the buttons in the toolbar.

      1. Sorry I haven't gotten back to you. There are ways we can lay this out to address this. Feel free to ping me on Slack and we'll talk about it! :)

    3. 
        
    hxqlin
    hxqlin
    Review request changed
    Change Summary:

    Revert attempts to change the Enable Markdown checkbox to a button and add a more robust implementation for toggling list syntax.

    Commit:
    057181aa978c1126ce8f7a75a71f0997ce0825b5
    4f57506f709277300f195691732553dea3d26cd0

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    hxqlin
    chipx86
    1. I'm excited to see this come together! Thanks for sticking with it :)

      This review looks large. Most of it are small tasks about complying with doc/syntax standards.

      There's a few bigger-ticket items:

      1. Updating the CSS to adhere to our CSS Component styles (I go into this and provide links below).
      2. Re-evaluating some regexes (we can talk about this in Slack if it helps).
      3. Placement of the toolbar.

      I'd knock out the easy things, update the change, and then we can talk about the bigger items.

    2. Show all issues

      I think it'd look nice to have this show up within the editor. Kind of like in Asana. By having it within the confines of the text area, it more directly connects it to the content.

      I don't know how hard this is. It'll have to happen before we release Review Board 5 for sure, but I know you have a full-time job these days. If this ends up being a lot to figure out, I'm okay committing without this, and moving this work to our plate.

    3. reviewboard/static/rb/css/common.less (Diff revision 4)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Since we're in the final stages of this, it'd be nice to move this into rb/css/ui/formatting-toolbar.less and to use our CSS component style and documentation conventions. You can see a good example of this in page-sidebar.less, and see docs here:

      1. Would the buttons then be .rb-o-bold-button, .rb-o-italic-button, etc. ? And is rb-c-formatting-toolbar__btn-group okay as is?

    4. Show all issues

      It's best not to override the function. Instead, let's bind it when we pass it to the constructor below.

    5. Show all issues

      To stay consistent, the pattern we normally use is:

      this._$toolbar.$el.appendTo(this.$el);
      
    6. Show all issues

      Two blank lines between top-level classes or functions.

    7. Show all issues

      Option arguments should be documented in an Option Args section, following Args. These work just like other arguments, listing names, types, and descriptions.

    8. Show all issues

      Can you have this wrap? You can separate it into a chain of jQuery statements.

    9. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
       
       
       
      Show all issues

      Element building should happen in render().

    10. Show all issues

      This can be one line, since we're not chaining.

    11. Show all issues

      We currently avoid for .. in, due to Babel's expensive, verbose transpilation. Can you convert this to a standard for loop?

    12. Show all issues

      Missing quotes around ${id}, to make the tag valid.

    13. Show all issues

      The .addClass should be indented 4 spaces.

    14. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
       
       
      Show all issues

      I think this can be consolidated a bit, and updated to be more explicit at the same time. How about:

      const $buttons = this.btnGroups[id].map(
          buttonInfo => this._makeButton(buttonInfo));
      
      this.$btnGroups.push($(`<div id="${id}"/>`)
          .addClass(...)
          .append($buttons));
      
    15. Show all issues

      Let's name buttonInfo options instead, and add an Option Args section so it's clear what's allowed by the caller.

      Also needs a Returns.

    16. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
      Show all issues

      We're largely moving to .on() instead of shortcuts like .click() for clarity (and to avoid some issues with edge cases that we've encountered in the past).

      This can also be combined as:

      $button.on('click', (_.isFunction(options.onClick)
                           ? options.onClick
                           : this[options.onClick].bind(this)));
      
    17. Show all issues

      For a setter, "Set the position of the cursor" would be more correct.

      This also isn't specific to emphasis.

    18. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
       
      Show all issues

      We fetch this._codeMirror multiple times. Let's pull it out into a variable to reduce lookups.

    19. Show all issues

      "Toggle"

    20. Show all issues

      A few notes:

      1. You'll need to escape these. You should use double backticks around these, instead of quotes. The backtick one is tricky. Just say "or a backtick for code."

      2. You mention * for italics, but the handler for the italic button uses _. We should say that in the docs here.

      3. Much of the content in this is probably most suitable for the main doc description.

    21. Show all issues

      I don't think the term "emphasis" is quite right. In Markdown, it can refer to bold/italic, but not strikethrough or code. How about toggleInlineTextFormat?

    22. Show all issues

      Let's pull this._codeMirror out into a local variable here as well. We access it a lot.

    23. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
      Show all issues

      The * should all align.

    24. Show all issues

      We don't use [..., ...] = ... syntax in our codebase, because Babel transforms that into some lengthy code. Let's instead pull this out as an array and then pull out the values from that.

    25. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
      Show all issues

      I want to dive into this regex with you, because I'm not confident it'll do what you want.

      Let's take a look at an example, using symbol='**' (as mentioned in the docs).

      1. ^[**]+|[**]+$
      2. **sometext**

      * isn't going to match the character *. In fact, it's not even valid regex by itself. What you'd need is \*\*, to get the literal characters.

      Even then, [\*\*] is equivalent to [\*], since [] is used to specify a set of possible matches for a single character. The generated regexes for bold are therefore the exact same as those for italic.

      What's the main purpose of this particular regex? To match tbe beginning or end of a line?

      If so, I'm not sure the | is going to work correctly here in all regex implementations without a containing parens (and I'm not sure the ^ or $ are guaranteed to work as part of that).

      This might be worth going into one-on-one on Slack.

      1. As mentioned on Slack, I was wrong about some of this (and right about other bits).

        [*] will match the character * in a regexp (unlike [.], which still means "any character"). The * does not get treated specially here.

        However, [**] is still equivalent to [*]. The string foo*bar will match foo[**]bar, for instance.

        If a list of possible charaters needs to be supplied for the same formatting (e.g., * and _), we should probably have those passed in as lists of strings, and turn them into, say, (\*|_), rather than using [...] syntax (note that you will have to escape in this case).

        The ^ and $ work fine in groups, from what I can tell, so I think that's okay. I haven't tested some older browsers, but that's okay, we're deprecating some older ones in the target release anyway. We should still put both sides of the | in parens, to make it clear.

    26. Show all issues

      The asterisks should align.

    27. Show all issues

      Formatting isn't quite right here. This should look like:

      const newSelection = selection.replace(
          new RegExp(...),
          '');
      
    28. Show all issues

      More cases of this regex. I count 3. Once we figure out what it should be, we should have a utility function that generates it, so that we don't have to repeat it.

    29. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
      Show all issues

      Asterisks should align.

    30. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Seeing inconsistency on indentation. Can you make sure this is correct all throughout?

    31. Show all issues

      Same note as above with the [..., ...] = ... syntax.

    32. Show all issues

      THis can be a single-line comment.

    33. Show all issues

      This can be a singline line comment.

    34. Show all issues

      This can be a single line comment.

    35. Show all issues

      This line is too long. Can you wrap the second parameter to the next line>?

    36. Show all issues

      No blank line here.

    37. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
       
       
       
      Show all issues

      This needs a "Returns" section.

    38. Show all issues

      "Find"

    39. Show all issues

      We access this._codeMirror many times. Let's pull it out into a variable.

    40. reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
       
       
       
       
       
       
       
       
      Show all issues

      Let's make this:

      for (let curToken = codeMirror.getTokenAt(cursorStart, true);
           curToken.string !== ' ' && groupStart.ch !== 0;
           curToken = codeMirror.getTokenAt(groupStart, true)) {
        groupStart.ch -= 1;
      }
      

      And same below.

      This better defines the scope and purpose of curToken and keeps the logic together as a single block of code with a clear intention.

    41. Show all issues

      You can use -= here.

    42. Show all issues

      You can use -=.

    43. Show all issues

      We can pull this._codeMirror out into a local variable.

    44. Show all issues

      "Insert"

    45. Show all issues

      "Markdown"

    46. Show all issues

      Let's pull this._codeMirror out into a local variable.

    47. Show all issues

      All arguments to a function should align.

    48. Show all issues

      This line is too long.

      Also, we prefer to keep logic out of the ${...} references in format strings. Let's calculate a value separately and then include it.

    49. Show all issues

      These lines are too long. Let's wrap:

      const precedingText =
          codeMirror.getLineTokens(...)
          .filter(...)
          ...
      
    50. Show all issues

      This line is too long. Let's do:

      codeMirror.setSelection({
          ch: ...,
          line: ...,
      });
      

      And use alphabetical order on the keys.

    51. Show all issues

      Same notes as above with the expression and line lengths.

    52. Show all issues

      We avoid for ... of syntax due to Babel transpiling this into an expensive block of code. Let's just use a standard for loop.

    53. 
        
    hxqlin
    hxqlin
    Review request changed
    Change Summary:
    • fix toolbar positioning
    • fix regex for asterisks
    • support both * and _ for italics
    • remove logic for adding new line characters when formatting lists to simplify both behaviour & implementation
    • add tests
    Description:
       

    This patch adds a markdown formatting toolbar for rich text editors.

    ~   The formatting-related buttons add the corresponding syntax to the
      ~ The buttons for inline formatting add the corresponding syntax to the
        selection if one exists, or just inserts the syntax and positions
        the cursor. This applies to the bold, italic, strikethrough, ordered
        and unordered list, and code buttons. If the selection begins and ends
        with the same syntax as the button being pressed, the syntax is removed.
        For example, if the bold button is pressed and the selection is already
        bolded, the bold syntax is removed.

       
       

    For the insert link button, if there is a selection, the selection

        becomes the text to display for the url. Otherwise, it inserts syntax
    ~   for a link with a text to display placeholder. For the upload image
    ~   button, it opens a dialog to upload an image. A reference to the image
    ~   is added to the editor.

      ~ for a link and positions the cursor where the text to display will go.
      ~ Positioning the cursor in a formatted link, selecting the text in the
      ~ link, or the either link will remove the formatting.

      +
      +

    For the upload image button, it opens a dialog to upload an image.

      + A reference to the image is added to the editor.

      +
      +

    For the ordered and unordered list buttons, numbers (1.) or dashes (-)

      + are appended to or removed from to the beginning of the current line.

    Testing Done:
    ~  

    Manual testing only so far.

      ~

    Unit tests for each type of formatting with:

      + - an empty selection
      + - cursor positioned in text
      + - unformatted text selection
      + - formatted text selection with text only, not including syntax
      + - formatted text selection including both text & syntax

    Commit:
    0cf11a5b41cce55c55ccb541f91e156d7060b643
    f1fc4f0ccabaf8b170d63a158da8ba57e35a120e

    Checks run (1 failed, 1 succeeded)

    flake8 passed.
    JSHint failed.

    JSHint

    hxqlin
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-6.x (3ffbade)