flake8
passed.
JSHint
failed.
Port $.inlineEditor to Backbone.
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
InlineEditorViewwhich does
everything that$.inlineEditorcan 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
$.inlineEditorto the new view.
Ports for the last uses will be coming in a later change.
- Ran js-tests.
- Manually tested all of the builtin review request fields that used inline
editors. - Tested multiline fields with both Markdown and plain text input and switching
between the two. - Tested dirty state calculations and attempting to leave the page with dirty,
unsaved editors.
| 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:
-
2b68e0d404ccca6cb30833dff4956ee66ea62719135961518313686f9f07c74dc7cf05ef2b53c294
- Diff:
-
Revision 2 (+1018 -193)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
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
inlineEditorover 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.
-
-
-
-
-
-
-
-
The
keyproperty doesn't look completely supported in Edge and appears completely unsupported in Safari for these keys. Can we go back to usingkeyCodeand the constants for now? -
-
-
-
-
-
This is referring to
outerHeightitself and not calling it, and isn't accessing the option correctly. Needs manual testing. -
-
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.
-
-
Original code normalized
initialValuefirst. The new code is doing it in the defaultisFieldDirty, butTextEditorViewdoesn't benefit from that. -
-
-
-
The original version also set
$el.data('text-editor'), which was accessed by other things. Might be worth adding that back? -
-
- Summary:
-
Port $.inlineEditor to backbone.Port $.inlineEditor to Backbone.
- Description:
-
~ The inline editor view is some of the first javascript code we wrote, and boy
~ 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
InlineEditorViewwhich does~ This change adds a new Backbone view called
InlineEditorViewwhich doeseverything that $.inlineEditorcan do and more. It provides all the optionsthat 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
$.inlineEditorto the new view.Ports for the last uses will be coming in a later change. - Commit:
-
135961518313686f9f07c74dc7cf05ef2b53c294460208b672ee6073c1d5361e41ba1de17573850e
- Diff:
-
Revision 3 (+1060 -193)
- Commit:
-
460208b672ee6073c1d5361e41ba1de17573850e9330d0a993f2b9a474a2d41477160845b35ffcb4
- Diff:
-
Revision 4 (+1060 -193)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
Some minor suggestions, this all looks pretty great :)
-
-
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. -
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? -
-
- Commit:
-
9330d0a993f2b9a474a2d41477160845b35ffcb4f27984ff160ecf708d4fc323ce54d44e6f4bf17e
- Diff:
-
Revision 5 (+1052 -193)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Commit:
-
f27984ff160ecf708d4fc323ce54d44e6f4bf17e836c452d8184254d7d0e9b960a040a99c1c2a4a4
- Diff:
-
Revision 6 (+1058 -193)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.