Rework review request fields.
Review Request #9144 — Created Aug. 22, 2017 and submitted
This change does a fairly large cleanup to the JavaScript side of the review
request fields. The old way was pretty hard-coded into the
ReviewRequestEditorView: each of the builtin fields had an entry which defined
some metadata about it, and the editor would scan through those, and then scan
through any remaining fields and pull (more limited) metadata out of the
markup.With the new model introduced in this change, each (editable) field has a
javascript view of its own which handles its interaction. There are base
classes for the existing types of fields, including plain text fields,
comma-separated multivalue fields, and multiline text fields. By default, each
of the field types defined infields.py
maps to one of these base classes,
which will fall back on the old behavior in order to maintain compatibility
with existing extensions that use them. If those field view types are
subclasses, they can add additional features such as autocomplete or
specialized saving mechanics. There's no longer any special casing between
"built-in" and "other" fields, aside from the fact that the built-in field
views specifyuseExtraData: false
.While I was in here, I cleaned up several related issues:
- Both the "close description" and "change description" fields used an element
with the ID#field_changedescription
.
- We no longer hard-code#field_changedescription
in the CSS.
- Some documentation updates and tweaks.There's still a lot more cleanup that can be done, moving functionality from
theReviewRequestEditorModel
andReviewRequestEditorView
into the field
classes, but this change seemed like it was big enough already.
- Ran unit tests.
- Ran js-tests.
- Manually tested all fields on the review request page, including rich text
and CSV-type fields that linkified their values in various ways. - Manually tested an extension that added various field types.
Description | From | Last Updated |
---|---|---|
The unit testing could use more fleshing out. I know we have existing tests for the built-in fields, but we … |
chipx86 | |
Col: 2 Expected a string and instead saw %. |
reviewbot | |
Col: 4 Expected ':' and instead saw 'load'. |
reviewbot | |
Col: 9 Expected a JSON value. |
reviewbot | |
Col: 9 Expected '}' and instead saw 'reviewtags'. |
reviewbot | |
Col: 9 Unrecoverable syntax error. (4% scanned). |
reviewbot | |
"JavaScript" Here and all the others. |
chipx86 | |
Description is intended 1 space too far. |
chipx86 | |
This kinda reads weird. Maybe: The way that the review request should be closed. This can be one of <values>. … |
chipx86 | |
So this broke when originally converted to ES6. The newline shouldn't have been there, as that adds unwanted whitespace to … |
chipx86 | |
Same here. There shouldn't be any newline involved. |
chipx86 | |
You can pass this.showBanner in directly. No need for the wrapping function. |
chipx86 | |
I'd like to avoid using for .. of. Babel turns it into a whole lot of pretty expensive JavaScript. Can … |
chipx86 | |
Missing trailing period. |
chipx86 | |
One-line summary. |
chipx86 | |
This should be using the standard function doc format. |
chipx86 | |
"Python", "JavaScript" |
chipx86 | |
Second sentence should be its own paragraph. One line summary. |
chipx86 | |
One-line summary, standard function doc format (since we don't have properties in JavaScript and shouldn't pretend). |
chipx86 | |
One-line summary. |
chipx86 | |
Needs to return this. |
chipx86 | |
What if we don't want auto-complete for the text field? Can this be optional? |
chipx86 | |
Can you put the property on the next line? Reads a bit better. |
chipx86 | |
This can use template literals. |
chipx86 | |
JSON.parse? |
brennie | |
Can you expand this to say more about what this does? How it formats it? |
chipx86 | |
Looks like we only use draft this once. We can get rid of that variable. |
chipx86 | |
For each of the built-in field descriptions, can you use the label (like "Branch", "Change Description", etc.) in the doc … |
chipx86 | |
"Array" |
chipx86 | |
Can you use data=[] in the function definition? |
chipx86 | |
"Array" |
chipx86 | |
Can this be data=[]? |
chipx86 | |
"Array" |
chipx86 | |
data=[]? |
chipx86 | |
"Array" |
chipx86 | |
data=[]? |
chipx86 | |
afterEach needs to do RB.DnDUploader.instance = null. Not sure whether this change came before my cleanup, so maybe this is … |
chipx86 | |
The include should be indented 2 more spaces. |
chipx86 | |
The include should be indented 1 more space. |
chipx86 | |
Col: 9 Expected '}' and instead saw 'reviewtags'. |
reviewbot | |
Col: 9 Expected a JSON value. |
reviewbot | |
Col: 9 Unrecoverable syntax error. (4% scanned). |
reviewbot | |
Col: 4 Expected ':' and instead saw 'load'. |
reviewbot | |
Col: 2 Expected a string and instead saw %. |
reviewbot | |
Still needed? |
chipx86 | |
Line's too long now. Review Bot's going to yell forever :/ Would be pretty cool to have dedent allow for … |
chipx86 | |
As discovered recently, we'll need to wrap the file in (function() { ... })(); |
chipx86 | |
This might be faster as: this._fieldName = this.fieldID.replace(/_(.)/g, (m, c) => c.toUpperCase()); Assuming you want to strip away all underscores … |
chipx86 | |
Missing "Returns" This should also go after useEditIconOnly. |
chipx86 | |
Typo in "Markdown". |
chipx86 | |
Col: 9 Unrecoverable syntax error. (5% scanned). |
reviewbot | |
Col: 9 Expected '}' and instead saw 'reviewtags'. |
reviewbot | |
Col: 9 Expected a JSON value. |
reviewbot | |
Col: 4 Expected ':' and instead saw 'load'. |
reviewbot | |
Col: 2 Expected a string and instead saw %. |
reviewbot | |
Col: 4 Expected ':' and instead saw 'load'. |
reviewbot | |
Col: 9 Expected a JSON value. |
reviewbot | |
Col: 9 Expected '}' and instead saw 'reviewtags'. |
reviewbot | |
Col: 9 Unrecoverable syntax error. (5% scanned). |
reviewbot | |
Col: 2 Expected a string and instead saw %. |
reviewbot |
- Change Summary:
-
Shuffle a bit of code up to base classes.
- Commit:
-
1205a491b672242e49ba4ac750364c6735d51e5de5d36b83ab0cf97039c84ef95dd072eb1dcce924
- Diff:
-
Revision 2 (+968 -634)
-
Looks great overall! Love where this is going.
A lot of my comments actually have to do with the docs not being consistent with standards. I've marked those. Also have comments about testing.
-
The unit testing could use more fleshing out. I know we have existing tests for the built-in fields, but we really should have tests for all the new registration support and the options for each field to ensure that the base foundation is working correctly. I know that's a lot more work, but it'll be worth it.
-
-
-
This kinda reads weird. Maybe:
The way that the review request should be closed. This can be one of <values>. This is only used if ``fieldName`` is ``closeDescription``.
-
So this broke when originally converted to ES6. The newline shouldn't have been there, as that adds unwanted whitespace to be beginning. Mind fixing it while you're here?
<%- describeText %>
should be flush up against the prior<label>
. -
-
-
I'd like to avoid using
for .. of
. Babel turns it into a whole lot of pretty expensive JavaScript. Can you use a standardfor .. in
or_.each()
instead? -
-
-
-
-
-
One-line summary, standard function doc format (since we don't have properties in JavaScript and shouldn't pretend).
-
-
-
-
-
-
-
-
For each of the built-in field descriptions, can you use the label (like "Branch", "Change Description", etc.) in the doc summary?
-
-
-
-
-
-
-
-
-
afterEach
needs to doRB.DnDUploader.instance = null
. Not sure whether this change came before my cleanup, so maybe this is working around issues fixed there, but for reference, see commit 528b268. -
-
-
- Commit:
-
e5d36b83ab0cf97039c84ef95dd072eb1dcce924a588a33c2be12b1f99a85a2335cdbaeb10ebf8ab
- Diff:
-
Revision 3 (+984 -636)
-
Only a few comments. Looks good to me otherwise. I'll do a final pass after this.
-
Line's too long now. Review Bot's going to yell forever :/ Would be pretty cool to have
dedent
allow for some character at the end of a line that says "Join the next line and ignore any leading spaces of that line."Maybe we can wrap right after
<%-
? Kinda ugly but... Less invasive. -
-
This might be faster as:
this._fieldName = this.fieldID.replace(/_(.)/g, (m, c) => c.toUpperCase());
Assuming you want to strip away all underscores and not just ones precedencing a letter.
-
-
- Commit:
-
a588a33c2be12b1f99a85a2335cdbaeb10ebf8ab5dc6c78eddba49221314a452f362df67aadd8c3d
- Diff:
-
Revision 4 (+984 -636)