[WIP] Review Composer: Comment UI

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

Malcolm
Review Board
master
b70de53...
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.

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

Loading file attachments...

  • 3
  • 0
  • 15
  • 0
  • 18
Description From Last Updated
hey, I was wondering if these blank lines exist in the actual file? praiseA praiseA
Alphabetize. brennie brennie
You should arrange them in alphabetical order bolariinwa bolariinwa
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
Review request changed

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.

-  
-  
  1. rb ui views InfoboxManagerView Target Events mouseenter First time for target
-  
  1. rb ui views InfoboxManagerView Target Events mouseleave Hides infobox
-  
  1. rb ui views InfoboxManagerView Infobox Events mouseleave Hides infobox
-  
  1. djblets configForms views ListView Manages items On remove
-  
  1. djblets configForms views TableView Manages rows On remove
-  
  1. 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

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
bolariinwa
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revision 4)
     
     
     
     
     
     
     

    You should arrange them in alphabetical order

  3. 
      
Loading...