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

Description From Last Updated

Since we're doing _super.remove below we don't need this.

brenniebrennie

Doing render() here will break things.

brenniebrennie

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

brenniebrennie

Col: 15 'commentList' is defined but never used.

reviewbotreviewbot

Col: 15 'tipsSection' is defined but never used.

reviewbotreviewbot

Col: 69 Missing semicolon.

reviewbotreviewbot

You don't need this becuase of the _super call below.

brenniebrennie

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

brenniebrennie

Col: 29 Missing semicolon.

reviewbotreviewbot

Col: 63 Missing semicolon.

reviewbotreviewbot

Col: 11 Missing semicolon.

reviewbotreviewbot

Col: 35 Missing semicolon.

reviewbotreviewbot

Alphabetize.

brenniebrennie

Alphabetize.

brenniebrennie

Alphabetize.

brenniebrennie

space between selector and {

brenniebrennie

These are not very semantic styles. Their names indicate what they do, not why they do it or what they ...

brenniebrennie

Space between selector and {

brenniebrennie

This should return this.

brenniebrennie

Col: 7 'DiffCommentView' is defined but never used.

reviewbotreviewbot

Same comment here re: inline styles.

brenniebrennie

Col: 7 'FileAttachmentCommentView' is defined but never used.

reviewbotreviewbot

Col: 7 'GeneralCommentView' is defined but never used.

reviewbotreviewbot

Col: 7 'ScreenshotCommentView' is defined but never used.

reviewbotreviewbot

We try to avoid inline style as much as possible. Is there a reason this can't be added to a ...

brenniebrennie

Col: 7 'HeaderFooterCommentView' is defined but never used.

reviewbotreviewbot

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

brenniebrennie

Col: 7 'ReviewOutline' is defined but never used.

reviewbotreviewbot

Col: 3 Missing semicolon.

reviewbotreviewbot

Same comment here re: inline styles.

brenniebrennie

Our templates are indented a single space, not four.

brenniebrennie

Space after ). Same elsewhere

brenniebrennie

This is also using inline style.

brenniebrennie

How about: this.listenTo( reviewDialogView._fileAttachmentCommentsCollection, 'add', comment => this._renderComment($reviewOutline, comment));

brenniebrennie

Since you're not doing anything with tipsSection, you can just do: this.$('#sidebar-tips-section') .append($(this.tipsTemplate({ ... })));

brenniebrennie

As a convention, we name jQuery objects $foo.

brenniebrennie

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

brenniebrennie

Since you aren't doing anything with $buttons afterwards, you can just do: this.$('#sidebar-tips-button') .append(...);

brenniebrennie

Our templates are indented a single space, not four.

brenniebrennie

Col: 7 'DiffCommentView' is defined but never used.

reviewbotreviewbot

Col: 7 'FileAttachmentCommentView' is defined but never used.

reviewbotreviewbot

Col: 7 'GeneralCommentView' is defined but never used.

reviewbotreviewbot

Col: 7 'ScreenshotCommentView' is defined but never used.

reviewbotreviewbot

Col: 7 'HeaderFooterCommentView' is defined but never used.

reviewbotreviewbot

Col: 7 'ReviewOutline' is defined but never used.

reviewbotreviewbot

Col: 3 Missing semicolon.

reviewbotreviewbot

Col: 7 'HeaderFooterCommentView' is defined but never used.

reviewbotreviewbot

Col: 58 Missing semicolon.

reviewbotreviewbot

Col: 58 Missing semicolon.

reviewbotreviewbot

Col: 58 Missing semicolon.

reviewbotreviewbot

Col: 58 Missing semicolon.

reviewbotreviewbot

Col: 7 'HeaderFooterCommentView' is defined but never used.

reviewbotreviewbot

Space after selector and before opening brace.

brenniebrennie

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

brenniebrennie

<%- text %>

brenniebrennie

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

brenniebrennie

<br> and it should be on the next line.

brenniebrennie

<%- lines %>

brenniebrennie

Unnecessary -- this is the default impl.

brenniebrennie

Here too

brenniebrennie

Space between ) and {

brenniebrennie

Should be indented one space.

brenniebrennie

<%- text %>

brenniebrennie

<br> on next line.

brenniebrennie

Unnecessary.

brenniebrennie

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

brenniebrennie

<%- text %>

brenniebrennie

<br> on next line.

brenniebrennie

Unnecessary.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

gettext()

brenniebrennie

Single quotes for strings

brenniebrennie

Text needs to be marked for translation with gettext, e.g. $('<span class="fa fa-plus"></span>') .text(gettext('Add a general comment'))

brenniebrennie

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

brenniebrennie

Undo

brenniebrennie

Comments should end in a period. However, I think the behaviour can be inferred from the name.

brenniebrennie

Comments should end in a period. Again, I don't know that this is necessary.

brenniebrennie

This file needs to be wrapped in an IIFE to not pollute global scope, e.g. (function() { const BaseCommentView = ...

brenniebrennie

Undo

brenniebrennie

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

brenniebrennie

No blank line here.

brenniebrennie

No blank line here.

brenniebrennie

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

brenniebrennie

No blank line here.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

There is an extra ", right before class

brenniebrennie

Needs a docstring.

brenniebrennie

Col: 22 Missing semicolon.

reviewbotreviewbot

Col: 5 'emptyDiffCommentsPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'emptyFileAttachmentCommentsPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'emptyGeneralCommentsPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'emptyScreenshotCommentsPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'diffCommentPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'fileAttachmentCommentPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'generalCommentPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'screenshotCommentPayload' is defined but never used.

reviewbotreviewbot

Col: 5 'origGeneralCommentsEnabled' is defined but never used.

reviewbotreviewbot

Col: 5 'dlg' is defined but never used.

reviewbotreviewbot

Col: 10 'createReviewDialog' is defined but never used.

reviewbotreviewbot

Col: 21 'ajaxData' is defined but never used.

reviewbotreviewbot

Col: 96 Missing semicolon.

reviewbotreviewbot

Col: 70 Missing semicolon.

reviewbotreviewbot

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

reviewbotreviewbot

Col: 56 Missing semicolon.

reviewbotreviewbot

Col: 70 Missing semicolon.

reviewbotreviewbot

W503 line break before binary operator

reviewbotreviewbot

W503 line break before binary operator

reviewbotreviewbot

W503 line break before binary operator

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