Review Composer: Comment UI
Review Request #10324 — Created Nov. 17, 2018 and updated
Information | |
---|---|
Malcolm | |
Review Board | |
master | |
50cf8e6... | |
Reviewers | |
reviewboard, students | |
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.
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.
Description | From | Last Updated |
---|---|---|
Seeing as you made visual changes to the review composer, it would be nice if you could add a screenshot. |
|
|
It looks like your other patch got rolled into this one. If your patch /r/10210/ is at branch-1 and this … |
|
|
|
||
hey, I was wondering if these blank lines exist in the actual file? |
|
|
Space after : |
|
|
Space after : |
|
|
Space after : |
|
|
Alphabetize. |
|
|
Space after : |
|
|
Space after : |
|
|
Why are we changing this default? This looks like it will break a bunch of views that use InlineEditorView. Where … |
|
|
This should probably use <%- id %>. |
|
|
Intentation is wrong. The <input> and <label> elements should be indented one space. |
|
|
These should all use <%- ... %>, which causes HTML escaping. |
|
|
This should only be indented 1 space relative to the <span> |
|
|
Undo this indent. |
|
|
Why are we removing the text here? If we should be removing it, you can just use '' instead of … |
|
|
Here too |
|
|
You should arrange them in alphabetical order |
|
|
Col: 80 Missing semicolon. |
![]() |
|
Col: 40 Expected '===' and instead saw '=='. |
![]() |
|
Col: 80 Missing semicolon. |
![]() |
|
Col: 40 Expected '===' and instead saw '=='. |
![]() |
|
Col: 80 Missing semicolon. |
![]() |
|
Col: 40 Expected '===' and instead saw '=='. |
![]() |
|
Col: 80 Missing semicolon. |
![]() |
|
Col: 40 Expected '===' and instead saw '=='. |
![]() |
Change Summary:
Added screenshot and ran unit tests.
Testing Done: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Added Files: |
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1) hey, I was wondering if these blank lines exist in the actual file?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+162 -84) |
Checks run (2 succeeded)
-
-
It looks like your other patch got rolled into this one.
If your patch /r/10210/ is at
branch-1
and this patch is atbranch-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 dorbt post-branch -r 10324 branch-2
. -
-
-
-
-
-
-
reviewboard/static/rb/js/ui/views/inlineEditorView.es6.js (Diff revision 2) 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. -
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2) This should probably use
<%- id %>
. -
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2) Intentation is wrong. The
<input>
and<label>
elements should be indented one space. -
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2) These should all use
<%- ... %>
, which causes HTML escaping. -
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2) This should only be indented 1 space relative to the
<span>
-
-
reviewboard/static/rb/js/views/reviewComposerView.es6.js (Diff revision 2) Why are we removing the text here?
If we should be removing it, you can just use
''
instead of callinggettext('')
. -
Change Summary:
Fixed some issues.
Testing Done: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+163 -84) |
Checks run (2 succeeded)
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 4) You should arrange them in alphabetical order
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+230 -85) |
Checks run (2 succeeded)
Change Summary:
Updated textEditorView.es6.js to include a dropdown menu for selecting Markdown for the text box.
Summary: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 6 (+267 -85) |
Checks run (2 succeeded)
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+317 -85) |
Checks run (1 failed, 1 succeeded)
JSHint
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 80 Missing semicolon.
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 40 Expected '===' and instead saw '=='.
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 80 Missing semicolon.
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 40 Expected '===' and instead saw '=='.
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 80 Missing semicolon.
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 40 Expected '===' and instead saw '=='.
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 80 Missing semicolon.
-
reviewboard/static/rb/js/views/tests/reviewComposerViewTests.js (Diff revision 7) Col: 40 Expected '===' and instead saw '=='.