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

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

Teresa Fan
Review Board
master
reviewboard, students

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.

Loading file attachments...

  • 1
  • 0
  • 44
  • 1
  • 46
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 ... Christian Hammond Christian Hammond
Review Bot
  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. 
      
Teresa Fan
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Teresa Fan
Teresa Fan
Review Bot
  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. 
      
Wang Jun Sun
Christian Hammond
  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. 
      
Teresa Fan
Teresa Fan
Review Bot
  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. 
      
Teresa Fan
Review Bot
  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. 
      
Teresa Fan
Review Bot
  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. 
      
Teresa Fan
Review Bot
  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. 
      
Christian Hammond
  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. 
      
Teresa Fan
Teresa Fan
Review Bot
  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. 
      
Teresa Fan
Review Bot
  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. 
      
Teresa Fan
Review Bot
  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. 
      
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Teresa Fan
Review Bot
  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. 
      
Teresa Fan
Review request changed

Status: Discarded

Loading...