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

Information

Reviewers

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