Redesign the issue summary table UI to speed up the process of addressing issues.

Review Request #6997 — Created Feb. 28, 2015 and discarded

Information

Review Board
master

Reviewers

Redesign the UI to make the issue summary table more useful, by:

  1. Making issue descriptions expand so that full content is viewable on click. This change allows the user to conveniently see full issue descriptions in-table, without having to click the description and be redirected to the comment itself.

A new div in review_issue_summary_table.html contains full comment text and is initially hidden.
The CSS class, 'issue-summary-collapsed,' is applied to this hidden div when the summary is collapsed. JS methods, onPreviewClicked and onFullClicked handle user clicks.

  1. Adding buttons to fix/drop issues in the issue summary table.

Each entry in the issue summary table now has a set of Fix and Drop buttons, visible when expanded, from which you can address the issue and the status of the issue will update everywhere. You can also continue to use the previously existing buttons in the review to fix, drop, or reopen, and the table will be updated accordingly.

Manual testing (expandable descriptions):
- Added long (past the 20 word cutoff mark) comments to a review request.
- Able to click the description in the issue summary table to see the full comment text.
- If the full text is being displayed, able to switch back to the 20 word version upon click.
(buttons):
- Clicking the Fix or Drop button when a description is expanded fades the issue out of the table and updates its status at the actual place in the review. It stays upon refreshing the page.

Added new commentIssueBar unit tests and ran js-tests. All passed.


Description From Last Updated

There's three elements on here (the text, the buttons, and the See Comment), and none of them align. We should …

chipx86chipx86

No blank line here.

brenniebrennie

Trailing whitespace.

brenniebrennie

Trailing whitespace.

brenniebrennie

Should begin with a capital

brenniebrennie

Use single quotes.

brenniebrennie

Use a multiline comment.

brenniebrennie

Trailing whitespace.

brenniebrennie

Use single quotes. Here and elsewhere.

brenniebrennie

Space :D

SU Sunxperous

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Make sure your docstrings follow the proper format: """One-line summary.""" or: """One-line summary. Multi-line description. """

chipx86chipx86

No need for the None parameter. It's implied.

chipx86chipx86

Everything in here is alphabetical but overflow/margin. Also, for margin, use 0 instead of 0px.

chipx86chipx86

Best to use either percentages, or to have a constant defined along with the other constant definitions, so that it's …

chipx86chipx86

A useful JavaScriptism is to do: !!this.options.inTable. That will keep true as true, but turn undefined, null, and false into …

chipx86chipx86

Blank line between these.

chipx86chipx86

The indentation needs to be 4 spaces, not 2.

chipx86chipx86

While here, can you remove the * in this line?

chipx86chipx86

Can you sort these alphabetically by selector?

chipx86chipx86

This needs a doc comment.

chipx86chipx86

Make sure to remove the trailing whitespace, here and below.

chipx86chipx86

Blank line between code and comments. All comments should follow sentence casing, with a trailing period.

chipx86chipx86

This is missing a var. It also should be called $hiddenDiv, since it's a jQuery element reference. Note that you …

chipx86chipx86

This is also missing a var, and needs to be named $link. These same comments all apply to the following …

chipx86chipx86

Best not to hard-code CSS here. Can we use class names instead? Same below where you set display: none. In …

chipx86chipx86

Needs a doc comment.

chipx86chipx86

Space after the comma.

chipx86chipx86

Missing a doc comment.

chipx86chipx86

Best to pull out $(this) into $this, so we don't have to wrap it more than once.

chipx86chipx86

If this shouldn't happen, we need to console.assert it.

chipx86chipx86

I don't believe you need to parseInt this. Can you check whether it comes back as an integer? I believe …

chipx86chipx86

This is really, really wordy, and hard to maintain and review. Please break it up into multiple lines where possible. …

chipx86chipx86

reviewtags and staticfiles are not used.

chipx86chipx86

Can you instead have display: none on issue-container? Also, each button label must be localized.

chipx86chipx86

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Docstrings should be of the form: """Single line summary. Multi-line description. """

brenniebrennie

Remove trailing whitespace.

brenniebrennie

Method doccomments should use multi-line comment syntax, e.g /* * Hide ... */

brenniebrennie

Method doccomments should use multi-line comment syntax, e.g /* * Hide ... */

brenniebrennie

Remove trailing whitespace.

brenniebrennie

Method doccomments should use multi-line comment syntax, e.g /* * Create ... */

brenniebrennie

Can we assign $(this) to a variable (e.g., $this) and use that?

brenniebrennie

Same here.

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
    
    
  2. 
      
TE
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/.DS_Store
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/.DS_Store
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
    
    
  2. 
      
brennie
  1. <p>You should add <code>.DS_Store</code> to your <code>~/.gitconfig</code>. Please remove the <code>.DS_Store</code> file from the review request.</p>

    1. I can't find a .gitconfig file, does making a .gitignore file work? And how do I remove the file from this review request? (or will it automatically be removed when I update it with my git commit, after I've added .DS_Store to a gitignore)

    2. You can configure a global .gitignore file: git config --global core.excludesfile /Users/yourself/.gitignore or see this StackOverflow thread. Then, I believe you will need to remove the existing .DS_Store with the command git rm --cached ./.DS_Store as it will not be removed automatically. In the future, no .DS_Store files will be added should you have the global .gitignore file with .DS_Store as an entry.

    3. Oops! I did mean /.gitignore. Sorry!

    4. Worked. Thanks to both of you!

  2. No blank line here.

  3. Trailing whitespace.

  4. Trailing whitespace.

  5. Should begin with a capital

  6. Use a multiline comment.

  7. Trailing whitespace.

  8. Use single quotes. Here and elsewhere.

  9. 
      
TE
TE
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/.DS_Store
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/.DS_Store
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
    
    
  2. 
      
SU
chipx86
  1. Can you provide screenshots?

    Also, make sure you're following our guidelines for the review request description: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

  2. 
      
TE
TE
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/commentIssueBarView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/commentIssueBarView.js
    
    
  2. 
      
TE
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/commentIssueBarView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/commentIssueBarView.js
    
    
  2. 
      
TE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/commentIssueBarView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/commentIssueBarView.js
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
TE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
chipx86
  1. 
      
  2. There's three elements on here (the text, the buttons, and the See Comment), and none of them align. We should strive to have them align as best as possible.

    1. I've since fixed that issue (I think this image was just a mockup) so that the buttons and See Comment are aligned, but personally I thought it looked better with those 2 elements indented. I'll attached 2 more photos, one of how it currently looks that way, and another with all 3 elements aligned.

  3. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 7)
     
     
     
     

    Make sure your docstrings follow the proper format:

    """One-line summary."""
    

    or:

    """One-line summary.
    
    Multi-line description.
    """
    
  4. No need for the None parameter. It's implied.

  5. reviewboard/static/rb/css/pages/reviews.less (Diff revision 7)
     
     
     
     
     
     

    Everything in here is alphabetical but overflow/margin.

    Also, for margin, use 0 instead of 0px.

  6. Best to use either percentages, or to have a constant defined along with the other constant definitions, so that it's easier to find and update this if we make major changes to the styling.

  7. A useful JavaScriptism is to do: !!this.options.inTable. That will keep true as true, but turn undefined, null, and false into false.

  8. Blank line between these.

  9. reviewboard/static/rb/js/views/commentIssueBarView.js (Diff revision 7)
     
     
     
     
     
     

    The indentation needs to be 4 spaces, not 2.

  10. While here, can you remove the * in this line?

  11. Can you sort these alphabetically by selector?

  12. This needs a doc comment.

  13. Make sure to remove the trailing whitespace, here and below.

  14. Blank line between code and comments.

    All comments should follow sentence casing, with a trailing period.

  15. This is missing a var. It also should be called $hiddenDiv, since it's a jQuery element reference.

    Note that you must have one var statement for all variables, comma-separated, one per line, and it must be at the top of the function.

    Also, is there a class name you can reference in the ancestry instead of doing parent().parent()? It'll help reduce breakages if we ever need to alter the DOM here.

    1. There is a class name, but since there is an element with that class name for each entry in the table, is there some way I can select the closest matching element? Having to extract the comment id and add it as a property of each of these elements seems a bit involved (but I'm happy to do it if that's the best option).

    2. jQuery has the .closest() selector which finds the closest ancestor element that maches. If the $el.parent().parent() element has the class .grandparent then you could do $el.closest('.grandparent').find('div.issue-full').

  16. This is also missing a var, and needs to be named $link.

    These same comments all apply to the following function.

  17. Best not to hard-code CSS here. Can we use class names instead?

    Same below where you set display: none.

    In general, though, if you need to hard-code CSS, you shouldn't modify style, but rather should .css('display', 'inline-block').

    1. jQuery fading in and out edits the display value inline, so it overrides any classes I add.

  18. Needs a doc comment.

  19. Space after the comma.

  20. Missing a doc comment.

  21. Best to pull out $(this) into $this, so we don't have to wrap it more than once.

    1. It's not working without the parentheses, is there something else I should be doing? $(this) is also used in line 255 (original code, not added by me).

    2. I believe Christrian means that you should do:

      var $this = $(this),
          dest = $(this).find('...'),
          ...;
      
  22. If this shouldn't happen, we need to console.assert it.

  23. I don't believe you need to parseInt this. Can you check whether it comes back as an integer? I believe .date() takes care of this for you.

    1. It comes back as a string without the parseInt, that was the bug that took me so long to figure out.

  24. This is really, really wordy, and hard to maintain and review. Please break it up into multiple lines where possible. You can use {% spaceless %}...{% endspaceless %} if there are issues with spaces causing problems.

    Also, note that the "See comment" must be localized.

  25. reviewtags and staticfiles are not used.

  26. reviewboard/templates/reviews/table_comment_issue.html (Diff revision 7)
     
     
     
     
     
     

    Can you instead have display: none on issue-container?

    Also, each button label must be localized.

    1. Each button needs to individually have display:none because the js code uses the issues status to individually determine which one(s) to show.
      I based this file off of the existing comment_issue.html, which I did not write, and I think the buttons also aren't localized there. Should I do both at the same time?

  27. 
      
TE
TE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
TE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
  2. Col: 1
     E302 expected 2 blank lines, found 1
    
  3. 
      
TE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
  2. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 10)
     
     
     
     

    Docstrings should be of the form:

    """Single line summary.
    
    Multi-line description.
    """
    
  3. Remove trailing whitespace.

  4. Method doccomments should use multi-line comment syntax, e.g

    /*
     * Hide ...
     */
    
  5. Method doccomments should use multi-line comment syntax, e.g

    /*
     * Hide ...
     */
    
  6. Remove trailing whitespace.

  7. Method doccomments should use multi-line comment syntax, e.g

    /*
     * Create ...
     */
    
  8. reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 10)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can we assign $(this) to a variable (e.g., $this) and use that?

  9. 
      
TE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/table_comment_issue.html
        reviewboard/static/rb/js/views/commentIssueBarView.js
        reviewboard/templates/reviews/review_issue_summary_table.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/rb/js/views/issueSummaryTableView.js
        reviewboard/static/rb/js/views/tests/commentIssueBarViewTests.js
    
    
  2. 
      
TE
Review request changed

Status: Discarded

Loading...