Summary

Review Composer: Comment UI

Review Request #10324 — Created Nov. 17, 2018 and updated Dec. 30, 2018, 11:45 a.m.

Information

Owner

Malcolm

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

50cf8e6...

Reviewers

Groups

reviewboard, students

People

Description

I have added some visual changes to emulate the style of
the Comment UI in the design document version of the Review
Composer. These are mostly CSS based. I used a similar solution
for the Ship it checkbox on the "Open an Issue" checkbox.
I also moved the "Edit" icon to the right of the screen through
CSS to emulate the look of the design document.

Added some code for the dropdown menu. Not finished.

Testing Done

I ran the JS unit tests and there weren't any errors.
The inclusion of the new code for the dropdown menu in textEditorView.es6.js caused some tests to fail.

Files

Issues

Description	From	Last Updated
Seeing as you made visual changes to the review composer, it would be nice if you could add a screenshot.	bolariinwa	Nov. 20, 2018, 12:53 p.m.
It looks like your other patch got rolled into this one. If your patch /r/10210/ is at branch-1 and this …	brennie	Dec. 1, 2018, 1:21 p.m.
	Malcolm	Dec. 14, 2018, 2:05 p.m.
hey, I was wondering if these blank lines exist in the actual file?	praiseA	Nov. 22, 2018, 5:46 p.m.
Space after :	brennie	Dec. 1, 2018, 1:22 p.m.
Space after :	brennie	Dec. 1, 2018, 1:22 p.m.
Space after :	brennie	Dec. 1, 2018, 1:22 p.m.
Alphabetize.	brennie	Nov. 24, 2018, 11:56 a.m.
Space after :	brennie	Dec. 1, 2018, 1:23 p.m.
Space after :	brennie	Dec. 1, 2018, 1:23 p.m.
Why are we changing this default? This looks like it will break a bunch of views that use InlineEditorView. Where …	brennie	Dec. 1, 2018, 1:25 p.m.
This should probably use <%- id %>.	brennie	Dec. 1, 2018, 1:25 p.m.
Intentation is wrong. The <input> and <label> elements should be indented one space.	brennie	Dec. 1, 2018, 1:28 p.m.
These should all use <%- ... %>, which causes HTML escaping.	brennie	Dec. 1, 2018, 1:28 p.m.
This should only be indented 1 space relative to the <span>	brennie	Dec. 1, 2018, 1:29 p.m.
Undo this indent.	brennie	Dec. 1, 2018, 1:30 p.m.
Why are we removing the text here? If we should be removing it, you can just use '' instead of …	brennie	Dec. 1, 2018, 1:31 p.m.
Here too	brennie	Dec. 1, 2018, 1:31 p.m.
You should arrange them in alphabetical order	bolariinwa	Dec. 4, 2018, 1:25 p.m.
Col: 80 Missing semicolon.	reviewbot	Dec. 30, 2018, 11:45 a.m.
Col: 40 Expected '===' and instead saw '=='.	reviewbot	Dec. 30, 2018, 11:45 a.m.
Col: 80 Missing semicolon.	reviewbot	Dec. 30, 2018, 11:45 a.m.
Col: 40 Expected '===' and instead saw '=='.	reviewbot	Dec. 30, 2018, 11:45 a.m.
Col: 80 Missing semicolon.	reviewbot	Dec. 30, 2018, 11:45 a.m.
Col: 40 Expected '===' and instead saw '=='.	reviewbot	Dec. 30, 2018, 11:45 a.m.
Col: 80 Missing semicolon.	reviewbot	Dec. 30, 2018, 11:45 a.m.
Col: 40 Expected '===' and instead saw '=='.	reviewbot	Dec. 30, 2018, 11:45 a.m.

flake8 passed.

JSHint passed.

Show all issues

Seeing as you made visual changes to the review composer, it would be nice if you could add a screenshot.

Malcolm Nov. 20, 2018, 7:15 a.m.
```
Thanks. I have added a screenshot.
```

Change Summary:

Added screenshot and ran unit tests.

Testing Done:

~		None yet.
	~	I ran the JS unit tests and there were 6 errors found.
	+
	+	rb ui views InfoboxManagerView Target Events mouseenter First time for target
	+	rb ui views InfoboxManagerView Target Events mouseleave Hides infobox
	+	rb ui views InfoboxManagerView Infobox Events mouseleave Hides infobox
	+	djblets configForms views ListView Manages items On remove
	+	djblets configForms views TableView Manages rows On remove
	+	djblets forms views ConditionValueFormFieldView Actions Delete a condition
	+
	+	I will work on resolving these errors.

Added Files:

Fix it!

reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)
Show all issues
```
hey, I was wondering if these blank lines exist in the actual file?
```

Commit:

-	1a69672d8d01006a8b83263018d1838cbad664a8
+	e77fb917c2f561bb5970e8a4bac6ea9a756aa9c0

Diff:

Revision 2 (+162 -84)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js
	reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

Show all issues

It looks like your other patch got rolled into this one.

If your patch /r/10210/ is at branch-1 and this patch is at branch-2, you can update this with the correct diff by doing: rbt post -r 10324 branch-1..branch-2.
If this patch is a single commit, you can just do rbt post-branch -r 10324 branch-2.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
Show all issues
```
Space after :
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
Show all issues
```
Space after :
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
Show all issues
```
Space after :
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
Show all issues
```
Alphabetize.
```
1. Malcolm Dec. 1, 2018, 1:33 p.m.
  Does the CSS class name need to alphabetized or does one of the fields?
reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
Show all issues
```
Space after :
```
reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
Show all issues
```
Space after :
```

reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2)

Show all issues

Why are we changing this default? This looks like it will break a bunch of views that use InlineEditorView.

Where we want multiline: false we should set that in the view options.

Malcolm Dec. 1, 2018, 1:38 p.m.

Thanks, that seems to have been what cause the errors in the JS unit tests.

reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2)
Show all issues
```
This should probably use <%- id %>.
```

reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2)

Show all issues

Intentation is wrong. The <input> and <label> elements should be indented one space.

reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2)
Show all issues
```
These should all use <%- ... %>, which causes HTML escaping.
```
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2)
Show all issues
```
This should only be indented 1 space relative to the <span>
```
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2)
Show all issues
```
Undo this indent.
```

reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2)

Show all issues

Why are we removing the text here?

If we should be removing it, you can just use '' instead of calling gettext('').

Malcolm Dec. 1, 2018, 1:31 p.m.

The notion project page's design document shows no text for the Header section so I removed the text. I replaced getText method with ''.

reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2)
Show all issues
```
Here too
```

Diff:

Revision 3 (+40 -3)

Show changes

	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js
	reviewboard/static/rb/js/views/reviewComposerView.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fixed some issues.

Testing Done:

~		I ran the JS unit tests and there were 6 errors found.
	~	I ran the JS unit tests and there weren't any errors.
-
-		rb ui views InfoboxManagerView Target Events mouseenter First time for target
-		rb ui views InfoboxManagerView Target Events mouseleave Hides infobox
-		rb ui views InfoboxManagerView Infobox Events mouseleave Hides infobox
-		djblets configForms views ListView Manages items On remove
-		djblets configForms views TableView Manages rows On remove
-		djblets forms views ConditionValueFormFieldView Actions Delete a condition
-
-		I will work on resolving these errors.

Commit:

-	e77fb917c2f561bb5970e8a4bac6ea9a756aa9c0
+	b70de532bb3e8fd629495ce580f7fee8d9b8b30d

Diff:

Revision 4 (+163 -84)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js
	reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

reviewboard/static/rb/css/pages/reviews.less (Diff revision 4)
Show all issues
```
You should arrange them in alphabetical order
```

Commit:

-	b70de532bb3e8fd629495ce580f7fee8d9b8b30d
+	511c13d61b94f9447b41f41037f81974f67f6afe

Diff:

Revision 5 (+230 -85)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js
	reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

Change Summary:

Updated textEditorView.es6.js to include a dropdown menu for selecting Markdown for the text box.

Summary:

-	[WIP] Review Composer: Comment UI
+	Review Composer: Comment UI

Description:

		I have added some visual changes to emulate the style of
		the Comment UI in the design document version of the Review
		Composer. These are mostly CSS based. I used a similar solution
		for the Ship it checkbox on the "Open an Issue" checkbox.
		I also moved the "Edit" icon to the right of the screen through
		CSS to emulate the look of the design document.
	+
	+	Added some code for the dropdown menu. Not finished.

Testing Done:

		I ran the JS unit tests and there weren't any errors.
	+
	+	The inclusion of the new code for the dropdown menu in textEditorView.es6.js caused some tests to fail.

Commit:

-	511c13d61b94f9447b41f41037f81974f67f6afe
+	bf39b3eeed9919f7b919fb6f332761b165046114

Diff:

Revision 6 (+267 -85)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js
	reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js
	reviewboard/static/rb/js/ui/views/textEditorView.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Added 4 unit tests to test the clicking the issue label at the header of each comment to toggle an issue on a comment.

Commit:

-	bf39b3eeed9919f7b919fb6f332761b165046114
+	50cf8e6cd2375f7a0d36bb89489e52a6621f234e

Diff:

Revision 7 (+317 -85)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.es6.js
	reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js
	reviewboard/static/rb/js/ui/views/textEditorView.es6.js
	reviewboard/static/rb/js/views/draftReviewBannerView.es6.js
	reviewboard/static/rb/js/views/reviewDialogView.es6.js
	2 more

Checks run (1 failed, 1 succeeded)

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 40
 Expected '===' and instead saw '=='.
```
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 40
 Expected '===' and instead saw '=='.
```
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 40
 Expected '===' and instead saw '=='.
```
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 80
 Missing semicolon.
```
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7)
Show all issues
```
Col: 40
 Expected '===' and instead saw '=='.
```

Review Composer: Comment UI

Information

Reviewers

I ran the JS unit tests and there weren't any errors.

The inclusion of the new code for the dropdown menu in textEditorView.es6.js caused some tests to fail.

Change Summary:

Testing Done:

I ran the JS unit tests and there were 6 errors found.

Added Files:

Commit:

Diff:

Checks run (2 succeeded)

Diff:

Checks run (2 succeeded)

Change Summary:

Testing Done:

I ran the JS unit tests and there were 6 errors found.

I ran the JS unit tests and there weren't any errors.

Commit:

Diff:

Checks run (2 succeeded)

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Summary:

Description:

Testing Done:

I ran the JS unit tests and there weren't any errors.

The inclusion of the new code for the dropdown menu in textEditorView.es6.js caused some tests to fail.

Commit:

Diff:

Checks run (2 succeeded)

Change Summary:

Commit:

Diff:

Checks run (1 failed, 1 succeeded)

JSHint