Summary:

Review Composer: Sidebar

Review Request #10225 — Created Oct. 11, 2018 and updated Dec. 14, 2018, 10:23 p.m.

Information
Owner:	bolariinwa
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Commit:	3de8456...
Reviewers
Groups:	reviewboard, students
People:

Description

The Review composer is the redesign of the review dialog to improve user
experience. The Review composer: sidebar serves as the sidebar for the
review composer.

The sidebar contains a button to create new general comments to replace the
add general comments button in the review dialog. A rendered list of comments
which will serve as a table of contents of reviews left in the review and a
tips and documentation section.

Testing Done

Added new JS unit tests to test functionality added. All JS tests pass.

Files

Issues

Description	From	Last Updated
Since we're doing _super.remove below we don't need this.	brennie	Oct. 26, 2018, 4:08 p.m.
Doing render() here will break things.	brennie	Oct. 26, 2018, 4:08 p.m.
This should be this.$el.append($generalCommentButton).	brennie	Oct. 26, 2018, 4:08 p.m.
Col: 15 'commentList' is defined but never used.	reviewbot	Nov. 2, 2018, 9:49 a.m.
Col: 15 'tipsSection' is defined but never used.	reviewbot	Nov. 2, 2018, 9:49 a.m.
Col: 69 Missing semicolon.	reviewbot	Nov. 2, 2018, 9:49 a.m.
You don't need this becuase of the _super call below.	brennie	Nov. 11, 2018, 7:32 p.m.
This should be in its own file. You will have to add the new file to staticbundles.py.	brennie	Nov. 11, 2018, 7:32 p.m.
Col: 29 Missing semicolon.	reviewbot	Nov. 15, 2018, 8:17 a.m.
Col: 63 Missing semicolon.	reviewbot	Nov. 15, 2018, 8:17 a.m.
Col: 11 Missing semicolon.	reviewbot	Nov. 15, 2018, 8:17 a.m.
Col: 35 Missing semicolon.	reviewbot	Nov. 16, 2018, 10:11 p.m.
Alphabetize.	brennie	Nov. 24, 2018, 2:32 p.m.
Alphabetize.	brennie	Nov. 24, 2018, 2:32 p.m.
Alphabetize.	brennie	Nov. 24, 2018, 6:30 p.m.
space between selector and {	brennie	Nov. 24, 2018, 6:30 p.m.
These are not very semantic styles. Their names indicate what they do, not why they do it or what they …	brennie	Nov. 28, 2018, noon
Space between selector and {	brennie	Nov. 24, 2018, 6:30 p.m.
This should return this.	brennie	Nov. 24, 2018, 6:30 p.m.
Col: 7 'DiffCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:30 p.m.
Same comment here re: inline styles.	brennie	Nov. 28, 2018, noon
Col: 7 'FileAttachmentCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:30 p.m.
Col: 7 'GeneralCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:30 p.m.
Col: 7 'ScreenshotCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:29 p.m.
We try to avoid inline style as much as possible. Is there a reason this can't be added to a …	brennie	Nov. 28, 2018, noon
Col: 7 'HeaderFooterCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 8:17 p.m.
If it is intentionally a no-op, you do not need to define it since the parent already does. Same elsewhere.	brennie	Nov. 24, 2018, 6:29 p.m.
Col: 7 'ReviewOutline' is defined but never used.	reviewbot	Nov. 24, 2018, 6:29 p.m.
Col: 3 Missing semicolon.	reviewbot	Nov. 24, 2018, 6:29 p.m.
Same comment here re: inline styles.	brennie	Nov. 28, 2018, noon
Our templates are indented a single space, not four.	brennie	Nov. 24, 2018, 6:28 p.m.
Space after ). Same elsewhere	brennie	Nov. 24, 2018, 2:31 p.m.
This is also using inline style.	brennie	Nov. 28, 2018, noon
How about: this.listenTo( reviewDialogView._fileAttachmentCommentsCollection, 'add', comment => this._renderComment($reviewOutline, comment));	brennie	Nov. 24, 2018, 2:31 p.m.
Since you're not doing anything with tipsSection, you can just do: this.$('#sidebar-tips-section') .append($(this.tipsTemplate({ ... })));	brennie	Nov. 24, 2018, 2:31 p.m.
As a convention, we name jQuery objects $foo.	brennie	Nov. 24, 2018, 2:31 p.m.
This should have $() wrapped around the template to convert create the DOM nodes to attach to the tree.	brennie	Nov. 24, 2018, 2:31 p.m.
Since you aren't doing anything with $buttons afterwards, you can just do: this.$('#sidebar-tips-button') .append(...);	brennie	Nov. 24, 2018, 2:31 p.m.
Our templates are indented a single space, not four.	brennie	Nov. 24, 2018, 6:28 p.m.
Col: 7 'DiffCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 7 'FileAttachmentCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 7 'GeneralCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 7 'ScreenshotCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 7 'HeaderFooterCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 8:16 p.m.
Col: 7 'ReviewOutline' is defined but never used.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 3 Missing semicolon.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 7 'HeaderFooterCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 8:16 p.m.
Col: 58 Missing semicolon.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 58 Missing semicolon.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 58 Missing semicolon.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 58 Missing semicolon.	reviewbot	Nov. 24, 2018, 6:28 p.m.
Col: 7 'HeaderFooterCommentView' is defined but never used.	reviewbot	Nov. 24, 2018, 8:16 p.m.
Space after selector and before opening brace.	brennie	Nov. 28, 2018, 8:10 p.m.
Is text HTML-escaped? Or do we know its safe HTML? If not we need to use <%- text %>.	brennie	Nov. 28, 2018, 8:11 p.m.
<%- text %>	brennie	Nov. 28, 2018, 8:33 p.m.
Same comment about <%= foo %> vs <%- foo %>.	brennie	Nov. 28, 2018, 8:11 p.m.
<br> and it should be on the next line.	brennie	Nov. 28, 2018, 8:33 p.m.
<%- lines %>	brennie	Nov. 28, 2018, 8:33 p.m.
Unnecessary -- this is the default impl.	brennie	Nov. 28, 2018, 8:33 p.m.
Here too	brennie	Nov. 28, 2018, 8:11 p.m.
Space between ) and {	brennie	Nov. 28, 2018, 8:11 p.m.
Should be indented one space.	brennie	Nov. 28, 2018, 8:11 p.m.
<%- text %>	brennie	Nov. 28, 2018, 8:33 p.m.
<br> on next line.	brennie	Nov. 28, 2018, 8:33 p.m.
Unnecessary.	brennie	Nov. 28, 2018, 8:33 p.m.
Same comment here vs <%= vs <%-.	brennie	Nov. 28, 2018, 8:11 p.m.
<%- text %>	brennie	Nov. 28, 2018, 8:33 p.m.
<br> on next line.	brennie	Nov. 28, 2018, 8:33 p.m.
Unnecessary.	brennie	Nov. 28, 2018, 8:33 p.m.
This has to be marked for translation with gettext(). Replace this with a template var and pass in the result …	brennie	Nov. 28, 2018, 8:32 p.m.
This has to be marked for translation with gettext(). Replace this with a template var and pass in the result …	brennie	Nov. 28, 2018, 8:32 p.m.
Same comment here about <%= vs <%-.	brennie	Nov. 28, 2018, 8:12 p.m.
We should avoid <b> in favour of <strong>, which is more semantic (see: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strong) However, since this is a title, …	brennie	Nov. 28, 2018, 8:33 p.m.
I don't know that we need an id selector here. class="tip-text" would be fine.	brennie	Nov. 28, 2018, 8:33 p.m.
gettext()	brennie	Nov. 28, 2018, 8:33 p.m.
Single quotes for strings	brennie	Nov. 28, 2018, 8:12 p.m.
Text needs to be marked for translation with gettext, e.g. $('<span class="fa fa-plus"></span>') .text(gettext('Add a general comment'))	brennie	Nov. 28, 2018, 8:13 p.m.
If you are going to use this for a multi-line string, use dedent, e.g. $(dedent` <span class="left-button fa fa-caret-left"></span> <span …	brennie	Nov. 28, 2018, 8:15 p.m.
Undo	brennie	Dec. 1, 2018, 7:51 p.m.
Comments should end in a period. However, I think the behaviour can be inferred from the name.	brennie	Dec. 1, 2018, 7:47 p.m.
Comments should end in a period. Again, I don't know that this is necessary.	brennie	Dec. 1, 2018, 7:47 p.m.
This file needs to be wrapped in an IIFE to not pollute global scope, e.g. (function() { const BaseCommentView = …	brennie	Dec. 1, 2018, 7:47 p.m.
Undo	brennie	Dec. 2, 2018, 4:52 p.m.
.sidebar-tips-button before .sidebar-tips-section.	brennie	Dec. 2, 2018, 4:52 p.m.
No blank line here.	brennie	Dec. 2, 2018, 4:52 p.m.
No blank line here.	brennie	Dec. 2, 2018, 4:52 p.m.
Move this to right before the block it is defined in.	brennie	Dec. 2, 2018, 4:52 p.m.
No blank line here.	brennie	Dec. 2, 2018, 4:53 p.m.
This does not interpolate variables, so it can just be a string literal.	brennie	Dec. 8, 2018, 4:01 p.m.
Instead of passing tips and current into the template, you can pass tip as tips[current] and use that instead.	brennie	Dec. 2, 2018, 4:51 p.m.
There is an extra ", right before class	brennie	Dec. 2, 2018, 4:50 p.m.
Needs a docstring.	brennie	Dec. 3, 2018, 9:11 p.m.
Col: 22 Missing semicolon.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'emptyDiffCommentsPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'emptyFileAttachmentCommentsPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'emptyGeneralCommentsPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'emptyScreenshotCommentsPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'diffCommentPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'fileAttachmentCommentPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'generalCommentPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'screenshotCommentPayload' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'origGeneralCommentsEnabled' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 5 'dlg' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 10 'createReviewDialog' is defined but never used.	reviewbot	Dec. 3, 2018, 9:14 p.m.
Col: 21 'ajaxData' is defined but never used.	reviewbot	Dec. 7, 2018, 6:23 a.m.
Col: 96 Missing semicolon.	reviewbot	Dec. 7, 2018, 6:23 a.m.
Col: 70 Missing semicolon.	reviewbot	Dec. 7, 2018, 6:23 a.m.
Col: 41 Expected '===' and instead saw '=='.	reviewbot	Dec. 7, 2018, 6:23 a.m.
Col: 56 Missing semicolon.	reviewbot	Dec. 7, 2018, 6:24 a.m.
Col: 70 Missing semicolon.	reviewbot	Dec. 7, 2018, 6:24 a.m.
W503 line break before binary operator	reviewbot	Dec. 10, 2018, 11:56 a.m.
W503 line break before binary operator	reviewbot	Dec. 10, 2018, 11:56 a.m.
W503 line break before binary operator	reviewbot	Dec. 10, 2018, 11:56 a.m.

flake8 passed.

JSHint passed.

Commit:

-	5fb2c2376b98734d72dbadd23daa7293de37f91c
+	04902898f25f64a43893b9830fdaab93165165ee

Review Composer: Sidebar

Commit:

Diff:

Checks run (2 succeeded)

Description:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Added Files:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Added Files:

Checks run (1 failed, 1 succeeded)

JSHint

Change Summary:

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Commit:

Diff:

Checks run (2 succeeded)

Description:

Commit:

Diff:

Added Files:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint

Commit:

Diff: