Review Composer: Sidebar

Review Request #10225 — Created Oct. 11, 2018 and updated

bolariinwa
Review Board
master
3de8456...
reviewboard, students

The Review composer is the redesign of the review dialog to improve user
experience. The Review composer: sidebar serves as the sidebar for the
review composer.

The sidebar contains a button to create new general comments to replace the
add general comments button in the review dialog. A rendered list of comments
which will serve as a table of contents of reviews left in the review and a
tips and documentation section.

Added new JS unit tests to test functionality added. All JS tests pass.

Loading file attachments...

  • 0
  • 0
  • 113
  • 0
  • 113
Description From Last Updated
bolariinwa
bolariinwa
bolariinwa
bolariinwa
brennie
  1. 
      
  2. Since we're doing _super.remove below we don't need this.

  3. Doing render() here will break things.

  4. This should be this.$el.append($generalCommentButton).

  5. 
      
bolariinwa
bolariinwa
bolariinwa
bolariinwa
bolariinwa
Review request changed

Commit:

-41a14e598e91c79ec3df033cec9e10579d6492cd
+a0cf5d26ce9ff762da87090594209b980d7b7b3f

Diff:

Revision 9 (+88 -15)

Show changes

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
bolariinwa
bolariinwa
brennie
  1. 
      
  2. You don't need this becuase of the _super call below.

  3. This should be in its own file. You will have to add the new file to staticbundles.py.

  4. 
      
bolariinwa
bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
bolariinwa
bolariinwa
bolariinwa
bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
bolariinwa
bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

brennie
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revision 22)
     
     
     
     

    Alphabetize.

  3. reviewboard/static/rb/css/pages/reviews.less (Diff revision 22)
     
     
     
     
     

    Alphabetize.

  4. reviewboard/static/rb/css/pages/reviews.less (Diff revision 22)
     
     
     
     
     

    Alphabetize.

  5. space between selector and {

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

    These are not very semantic styles. Their names indicate what they do, not why they do it or what they are styling. Can you update these with more descriptive names?

    e.g. why do we want .color ? Is it a header? If so then name it .header.

  7. Space between selector and {

  8. This should return this.

  9. Same comment here re: inline styles.

  10. We try to avoid inline style as much as possible. Is there a reason this can't be added to a .sidebar-general-comments rule in the CSS?

    1. The styles can be placed under a class. I used the inline style so I could quickly check the effect of a style.

  11. If it is intentionally a no-op, you do not need to define it since the parent already does. Same elsewhere.

  12. Same comment here re: inline styles.

  13. reviewboard/static/rb/js/views/reviewComposerSidebarView.es6.js (Diff revision 22)
     
     
     
     
     
     
     
     
     
     
     

    Our templates are indented a single space, not four.

  14. Space after ). Same elsewhere

  15. This is also using inline style.

  16. How about:

            this.listenTo(
                reviewDialogView._fileAttachmentCommentsCollection,
                'add',
                comment => this._renderComment($reviewOutline, comment));
    
  17. Since you're not doing anything with tipsSection, you can just do:

    this.$('#sidebar-tips-section')
        .append($(this.tipsTemplate({
            ...
        })));
    
  18. As a convention, we name jQuery objects $foo.

  19. This should have $() wrapped around the template to convert create the DOM nodes to attach to the tree.

  20. Since you aren't doing anything with $buttons afterwards, you can just do:

    this.$('#sidebar-tips-button')
        .append(...);
    
  21. reviewboard/static/rb/js/views/reviewDialogView.es6.js (Diff revision 22)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Our templates are indented a single space, not four.

  22. 
      
bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
bolariinwa
bolariinwa
brennie
  1. 
      
  2. Space after selector and before opening brace.

  3. Is text HTML-escaped? Or do we know its safe HTML? If not we need to use <%- text %>.

  4. Same comment about <%= foo %> vs <%- foo %>.

  5. Should be indented one space.

  6. Same comment here vs <%= vs <%-.

  7. This has to be marked for translation with gettext(). Replace this with a template var and pass in the result of gettext() when using the template.

  8. This has to be marked for translation with gettext(). Replace this with a template var and pass in the result of gettext() when using the template.

  9. Same comment here about <%= vs <%-.

  10. Single quotes for strings

  11. Text needs to be marked for translation with gettext, e.g.

    $('<span class="fa fa-plus"></span>')
        .text(gettext('Add a general comment'))
    
  12. If you are going to use this for a multi-line string, use dedent, e.g.

    $(dedent`
        <span class="left-button fa fa-caret-left"></span>
        <span class="right-button ra fa-caret-right"></span>
    `);
    
  13. 
      
bolariinwa
brennie
  1. 
      
  2. <br> and it should be on the next line.

  3. Unnecessary -- this is the default impl.

  4. <br> on next line.

    1. Should <br> be on it's own line

  5. Unnecessary.

  6. <br> on next line.

  7. Unnecessary.

  8. We should avoid <b> in favour of <strong>, which is more semantic (see: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strong)

    However, since this is a title, it should be a <h1>, <h2>, etc. element.

    Also this needs to be gettext()-ed.

  9. I don't know that we need an id selector here. class="tip-text" would be fine.

  10. 
      
bolariinwa
bolariinwa
bolariinwa
bolariinwa
brennie
  1. 
      
  2. Comments should end in a period.

    However, I think the behaviour can be inferred from the name.

  3. Comments should end in a period.

    Again, I don't know that this is necessary.

  4. This file needs to be wrapped in an IIFE to not pollute global scope, e.g.

    (function() {
    
    
    const BaseCommentView = Backbone.View.extend({...});
    
    // ..
    
    RB.ReviewComposerSidebar = Backbone.View.extend({...});
    
    })();
    

    This keeps all the variables in a function scope, so they don't escape into global scope (i.e, onto window).

  5. 
      
bolariinwa
brennie
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revision 34)
     
     
     
     
     
     
     
     
     
     
     
     
     

    .sidebar-tips-button before .sidebar-tips-section.

  3. Move this to right before the block it is defined in.

  4. This does not interpolate variables, so it can just be a string literal.

    1. when you say string literal, do you mean just wrapping it in a ''

  5. Instead of passing tips and current into the template, you can pass tip as tips[current] and use that instead.

  6. There is an extra ", right before class

  7. Needs a docstring.

  8. 
      
bolariinwa
bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

bolariinwa
bolariinwa
bolariinwa
bolariinwa
Review request changed
bolariinwa
bolariinwa
bolariinwa
bolariinwa
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

bolariinwa
bolariinwa
Review request changed
Loading...