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. 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. hey, I was wondering if these blank lines exist in the actual file?

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

  4. Space after :

  5. Space after :

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

    Alphabetize.

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

  7. Space after :

  8. Space after :

  9. 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. This should probably use <%- id %>.

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

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

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

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

  14. 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 ''.

  15. 
      
Malcolm
Malcolm
bolariinwa
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revision 4)
     
     
     
     
     
     
     

    You should arrange them in alphabetical order

  3. 
      
Malcolm
Malcolm
Malcolm
Malcolm
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

Loading...