JSHint
-
reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js (Diff revision 1) Show all issues
Review Request #9223 — Created Sept. 26, 2017 and submitted
The inline editor view is some of the first JavaScript code we wrote, and boy
does it show. It was built as a jQuery-UI component, and has grown over the
years to have a huge number of options. Unfortunately, the only extensibility
it provided was the ability to render a custom element when in multiline mode.
This was enough to let us add in the rich-text CodeMirror editor, but going
further was out of the question.This change adds a new Backbone view called
InlineEditorView
which does
everything that$.inlineEditor
can do and more. It provides all the options
that the jQuery-UI implementation does, but can also be subclassed to implement
even more complex behavior or replace the text editors with other input types
(including things as fancy as Selectize).This updates most (but not quite all) uses of
$.inlineEditor
to the new view.
Ports for the last uses will be coming in a later change.
Description | From | Last Updated |
---|---|---|
"Backbone" and "JavaScript" in the summary/description. |
chipx86 | |
Can this live in Djblets? Seems a more natural place for it, given that's where the prior version lives. I'd … |
chipx86 | |
Col: 56 ['summary'] is better written in dot notation. |
reviewbot | |
Class is missing a doc comment. |
chipx86 | |
This is missing support for useEditIconOnly and the logic around it. |
chipx86 | |
Backbone.View.prototype.initialize is guaranteed to be a no-op. We don't need to call it. |
chipx86 | |
Looks like an old type. |
chipx86 | |
Let's do .hide() first. It'll save some churn. |
chipx86 | |
Let's .hide() first here too. |
chipx86 | |
Instead of an extra function, can we pass this.submit.bind(this) and this.cancel.bind(this)? |
chipx86 | |
The key property doesn't look completely supported in Edge and appears completely unsupported in Safari for these keys. Can we … |
chipx86 | |
/** -> /* |
chipx86 | |
Blank line between these. |
chipx86 | |
Can we use .bind(this)? |
chipx86 | |
Can we take options instead, for future enhancement? |
chipx86 | |
Same here. |
chipx86 | |
This is referring to outerHeight itself and not calling it, and isn't accessing the option correctly. Needs manual testing. |
chipx86 | |
This no longer recalculates the dirty state in this method. Was that not needed? |
chipx86 | |
Logic here is different from what we have in the original code. This isn't doing the throttle anymore, which was … |
chipx86 | |
This should go below public methods. |
chipx86 | |
Original code normalized initialValue first. The new code is doing it in the default isFieldDirty, but TextEditorView doesn't benefit from … |
chipx86 | |
Can we call this isDirty()? |
chipx86 | |
Can you compare to 0? |
chipx86 | |
Missing docs. |
chipx86 | |
The original version also set $el.data('text-editor'), which was accessed by other things. Might be worth adding that back? |
chipx86 | |
buttons used to be public, and now it's private. Can we go ahead and make this $buttons? |
chipx86 | |
Instead of accessing a private variable, can we make $field public? |
chipx86 | |
Col: 63 Missing semicolon. |
reviewbot | |
Can we call it animationSpeedMS? The abbreviation doesn't save much. |
brennie | |
Since this isn't a closure over variables, we can pull it out into a function and just pass it in … |
brennie | |
Should this have aria-hidden="true" since its a visual cue only and we don't want screen-readers to attempt to read the … |
brennie | |
Any reason we don't set this up in this.events ? Or in delegateEvents ? |
brennie | |
Should we pul this out into this.$window in initialize? |
brennie | |
Might as well make this one line. |
chipx86 | |
This can be one line (no need for {}) |
chipx86 | |
You can use this._updateDirtyState.bind(this) instead, remove an unnecessary anonymous function. |
chipx86 |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+1018 -193) |
Nice cleanup. I went through each function and compared implementations, and there are some things that are missing or inconsistent. Given the fixes that have gone into
inlineEditor
over time, I'd like to make sure they're as consistent as possible. Made notes of what I saw.
Can this live in Djblets? Seems a more natural place for it, given that's where the prior version lives. I'd also eventually like to use this in Splat.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Class is missing a doc comment.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
This is missing support for
useEditIconOnly
and the logic around it.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Backbone.View.prototype.initialize
is guaranteed to be a no-op. We don't need to call it.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Let's do
.hide()
first. It'll save some churn.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Let's
.hide()
first here too.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Instead of an extra function, can we pass
this.submit.bind(this)
andthis.cancel.bind(this)
?
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
The
key
property doesn't look completely supported in Edge and appears completely unsupported in Safari for these keys. Can we go back to usingkeyCode
and the constants for now?
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Blank line between these.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Can we take
options
instead, for future enhancement?
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
This is referring to
outerHeight
itself and not calling it, and isn't accessing the option correctly. Needs manual testing.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
This no longer recalculates the dirty state in this method. Was that not needed?
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Logic here is different from what we have in the original code. This isn't doing the throttle anymore, which was added intentionally. (It was also doing a
_.defer()
, but this was prior to the_.throttle()
, and may not be needed).This should also go below public methods.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
This should go below public methods.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Original code normalized
initialValue
first. The new code is doing it in the defaultisFieldDirty
, butTextEditorView
doesn't benefit from that.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
Can we call this
isDirty()
?
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) |
---|
The original version also set
$el.data('text-editor')
, which was accessed by other things. Might be worth adding that back?
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2) |
---|
buttons
used to be public, and now it's private. Can we go ahead and make this$buttons
?
reviewboard/static/rb/js/views/reviewRequestFieldViews.es6.js (Diff revision 2) |
---|
Instead of accessing a private variable, can we make
$field
public?
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+1060 -193) |
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 3) |
---|
Col: 63 Missing semicolon.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+1060 -193) |
Some minor suggestions, this all looks pretty great :)
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 4) |
---|
Can we call it
animationSpeedMS
? The abbreviation doesn't save much.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 4) |
---|
Since this isn't a closure over variables, we can pull it out into a function and just pass it in instead of recreating it every time we make a new
InlineEditorView
.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 4) |
---|
Should this have
aria-hidden="true"
since its a visual cue only and we don't want screen-readers to attempt to read the button?
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 4) |
---|
Any reason we don't set this up in
this.events
? Or indelegateEvents
?
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 4) |
---|
Should we pul this out into
this.$window
ininitialize
?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+1052 -193) |
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 5) |
---|
Might as well make this one line.
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 5) |
---|
This can be one line (no need for
{}
)
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 5) |
---|
You can use
this._updateDirtyState.bind(this)
instead, remove an unnecessary anonymous function.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+1058 -193) |