Review Composer: Comment UI

Review Request #10324 — Created Nov. 17, 2018 and updated

Information

Review Board
master
50cf8e6...

Reviewers

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.

bolariinwabolariinwa

It looks like your other patch got rolled into this one. If your patch /r/10210/ is at branch-1 and this …

brenniebrennie

MalcolmMalcolm

hey, I was wondering if these blank lines exist in the actual file?

praiseApraiseA

Space after :

brenniebrennie

Space after :

brenniebrennie

Space after :

brenniebrennie

Alphabetize.

brenniebrennie

Space after :

brenniebrennie

Space after :

brenniebrennie

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

brenniebrennie

This should probably use <%- id %>.

brenniebrennie

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

brenniebrennie

These should all use <%- ... %>, which causes HTML escaping.

brenniebrennie

This should only be indented 1 space relative to the <span>

brenniebrennie

Undo this indent.

brenniebrennie

Why are we removing the text here? If we should be removing it, you can just use '' instead of …

brenniebrennie

Here too

brenniebrennie

You should arrange them in alphabetical order

bolariinwabolariinwa

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 40 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 40 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 40 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 40 Expected '===' and instead saw '=='.

reviewbotreviewbot
bolariinwa
  1. 
      
  2. Show all issues

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

    1. Thanks. I have added a screenshot.

  3. 
      
Malcolm
praiseA
  1. 
      
  2. Show all issues

    hey, I was wondering if these blank lines exist in the actual file?

  3. 
      
Malcolm
brennie
  1. 
      
  2. 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.

  3. Show all issues

    Space after :

  4. Show all issues

    Space after :

  5. Show all issues

    Space after :

  6. reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
     
     
     
     
     
    Show all issues

    Alphabetize.

    1. Does the CSS class name need to alphabetized or does one of the fields?

  7. Show all issues

    Space after :

  8. Show all issues

    Space after :

  9. 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.

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

  10. Show all issues

    This should probably use <%- id %>.

  11. 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.

  12. Show all issues

    These should all use <%- ... %>, which causes HTML escaping.

  13. Show all issues

    This should only be indented 1 space relative to the <span>

  14. Show all issues

    Undo this indent.

  15. Show all issues

    Why are we removing the text here?

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

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

  16. Show all issues

    Here too

  17. 
      
Malcolm
Malcolm
bolariinwa
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    You should arrange them in alphabetical order

  3. 
      
Malcolm
Malcolm
  1. 
      
  2. 
      
  3. Show all issues
    
      
  4. 
      
Malcolm
Malcolm
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint