JSHint
-
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 1) Show all issues
Review Request #11004 — Created May 15, 2020 and updated
Information | |
---|---|
hxqlin | |
Review Board | |
master | |
f1fc4f0... | |
Reviewers | |
reviewboard | |
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 … |
|
|
Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _) |
![]() |
|
This bit looks a bit weird; it's for the upload image button, which is different from the other buttons because … |
|
|
The button that toggles the rich text attribute is a bit tangled up in inlineEditorView so I wasn't sure how … |
|
|
Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _) |
![]() |
|
Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _) |
![]() |
|
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 … |
|
|
It's best not to override the function. Instead, let's bind it when we pass it to the constructor below. |
|
|
To stay consistent, the pattern we normally use is: this._$toolbar.$el.appendTo(this.$el); |
|
|
Two blank lines between top-level classes or functions. |
|
|
Option arguments should be documented in an Option Args section, following Args. These work just like other arguments, listing names, … |
|
|
Can you have this wrap? You can separate it into a chain of jQuery statements. |
|
|
Element building should happen in render(). |
|
|
This can be one line, since we're not chaining. |
|
|
We currently avoid for .. in, due to Babel's expensive, verbose transpilation. Can you convert this to a standard for … |
|
|
Missing quotes around ${id}, to make the tag valid. |
|
|
The .addClass should be indented 4 spaces. |
|
|
I think this can be consolidated a bit, and updated to be more explicit at the same time. How about: … |
|
|
Let's name buttonInfo options instead, and add an Option Args section so it's clear what's allowed by the caller. Also … |
|
|
We're largely moving to .on() instead of shortcuts like .click() for clarity (and to avoid some issues with edge cases … |
|
|
For a setter, "Set the position of the cursor" would be more correct. This also isn't specific to emphasis. |
|
|
We fetch this._codeMirror multiple times. Let's pull it out into a variable to reduce lookups. |
|
|
"Toggle" |
|
|
A few notes: You'll need to escape these. You should use double backticks around these, instead of quotes. The backtick … |
|
|
I don't think the term "emphasis" is quite right. In Markdown, it can refer to bold/italic, but not strikethrough or … |
|
|
Let's pull this._codeMirror out into a local variable here as well. We access it a lot. |
|
|
The * should all align. |
|
|
We don't use [..., ...] = ... syntax in our codebase, because Babel transforms that into some lengthy code. Let's … |
|
|
I want to dive into this regex with you, because I'm not confident it'll do what you want. Let's take … |
|
|
The asterisks should align. |
|
|
Formatting isn't quite right here. This should look like: const newSelection = selection.replace( new RegExp(...), ''); |
|
|
More cases of this regex. I count 3. Once we figure out what it should be, we should have a … |
|
|
Asterisks should align. |
|
|
Seeing inconsistency on indentation. Can you make sure this is correct all throughout? |
|
|
Same note as above with the [..., ...] = ... syntax. |
|
|
THis can be a single-line comment. |
|
|
This can be a singline line comment. |
|
|
This can be a single line comment. |
|
|
This line is too long. Can you wrap the second parameter to the next line>? |
|
|
No blank line here. |
|
|
This needs a "Returns" section. |
|
|
"Find" |
|
|
We access this._codeMirror many times. Let's pull it out into a variable. |
|
|
Let's make this: for (let curToken = codeMirror.getTokenAt(cursorStart, true); curToken.string !== ' ' && groupStart.ch !== 0; curToken = codeMirror.getTokenAt(groupStart, … |
|
|
You can use -= here. |
|
|
You can use -=. |
|
|
We can pull this._codeMirror out into a local variable. |
|
|
"Insert" |
|
|
"Markdown" |
|
|
Let's pull this._codeMirror out into a local variable. |
|
|
All arguments to a function should align. |
|
|
This line is too long. Also, we prefer to keep logic out of the ${...} references in format strings. Let's … |
|
|
These lines are too long. Let's wrap: const precedingText = codeMirror.getLineTokens(...) .filter(...) ... |
|
|
This line is too long. Let's do: codeMirror.setSelection({ ch: ..., line: ..., }); And use alphabetical order on the keys. |
|
|
Same notes as above with the expression and line lengths. |
|
|
We avoid for ... of syntax due to Babel transpiling this into an expensive block of code. Let's just use … |
|
|
Col: 23 Invalid regular expression. |
![]() |
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.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 1) |
---|
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 fromRB.TextEditorView
but also needs access to theinput
element to get the file. There must be a nicer way to do it but I'm not sure how.
Change the markdown checkbox to a button that toggles markdown in the field when clicked.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+462 -8) |
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 2) |
---|
Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _)
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
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.
Revert attempts to change the
Enable Markdown
checkbox to a button and add a more robust implementation for toggling list syntax.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+510 -2) |
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 3) |
---|
Col: 36 Functions declared within loops referencing an outer scoped variable may lead to confusing semantics. ($, _)
Updated implementation for emphasis syntax and added demo videos of current state
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+594 -2) |
||||
Removed Files: |
|||||
Added Files: |
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:
- Updating the CSS to adhere to our CSS Component styles (I go into this and provide links below).
- Re-evaluating some regexes (we can talk about this in Slack if it helps).
- Placement of the toolbar.
I'd knock out the easy things, update the change, and then we can talk about the bigger items.
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.
reviewboard/static/rb/css/common.less (Diff revision 4) |
---|
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 inpage-sidebar.less
, and see docs here:
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
It's best not to override the function. Instead, let's bind it when we pass it to the constructor below.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
To stay consistent, the pattern we normally use is:
this._$toolbar.$el.appendTo(this.$el);
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Two blank lines between top-level classes or functions.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Option arguments should be documented in an
Option Args
section, followingArgs
. These work just like other arguments, listing names, types, and descriptions.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Can you have this wrap? You can separate it into a chain of jQuery statements.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Element building should happen in
render()
.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
This can be one line, since we're not chaining.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
We currently avoid
for .. in
, due to Babel's expensive, verbose transpilation. Can you convert this to a standardfor
loop?
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Missing quotes around
${id}
, to make the tag valid.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
The
.addClass
should be indented 4 spaces.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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));
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Let's name
buttonInfo
options
instead, and add anOption Args
section so it's clear what's allowed by the caller.Also needs a
Returns
.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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)));
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
For a setter, "Set the position of the cursor" would be more correct.
This also isn't specific to emphasis.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
We fetch
this._codeMirror
multiple times. Let's pull it out into a variable to reduce lookups.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
A few notes:
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."
You mention
*
for italics, but the handler for the italic button uses_
. We should say that in the docs here.Much of the content in this is probably most suitable for the main doc description.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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
?
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Let's pull
this._codeMirror
out into a local variable here as well. We access it a lot.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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).
^[**]+|[**]+$
**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.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
The asterisks should align.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Formatting isn't quite right here. This should look like:
const newSelection = selection.replace( new RegExp(...), '');
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Seeing inconsistency on indentation. Can you make sure this is correct all throughout?
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Same note as above with the
[..., ...] = ...
syntax.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
THis can be a single-line comment.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
This can be a singline line comment.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
This can be a single line comment.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
This line is too long. Can you wrap the second parameter to the next line>?
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
This needs a "Returns" section.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
We access
this._codeMirror
many times. Let's pull it out into a variable.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
We can pull
this._codeMirror
out into a local variable.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Let's pull
this._codeMirror
out into a local variable.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
All arguments to a function should align.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
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.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
These lines are too long. Let's wrap:
const precedingText = codeMirror.getLineTokens(...) .filter(...) ...
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
This line is too long. Let's do:
codeMirror.setSelection({ ch: ..., line: ..., });
And use alphabetical order on the keys.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
Same notes as above with the expression and line lengths.
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4) |
---|
We avoid
for ... of
syntax due to Babel transpiling this into an expensive block of code. Let's just use a standard for loop.
Fix most docs/syntax issues and reposition toolbar inside the editor
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+643 -2) |
||||
Added Files: |
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+1377) |
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 6) |
---|
Col: 23 Invalid regular expression.