Redesign the issue summary table UI to speed up the process of addressing issues.
Review Request #6997 — Created Feb. 28, 2015 and discarded
| Information | |
|---|---|
| terfan | |
| Review Board | |
| master | |
| Reviewers | |
| reviewboard, students | |
Redesign the UI to make the issue summary table more useful, by:
- 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.
- 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 … |
 chipx86
|
|
|
No blank line here. |
 brennie
|
|
|
Trailing whitespace. |
brennie
|
|
|
Trailing whitespace. |
brennie
|
|
|
Should begin with a capital |
brennie
|
|
|
Use single quotes. |
brennie
|
|
|
Use a multiline comment. |
brennie
|
|
|
Trailing whitespace. |
brennie
|
|
|
Use single quotes. Here and elsewhere. |
brennie
|
|
|
Space :D |
SU Sunxperous | |
|
Col: 1 E302 expected 2 blank lines, found 1 |
 reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Make sure your docstrings follow the proper format: """One-line summary.""" or: """One-line summary. Multi-line description. """ |
chipx86
|
|
|
No need for the None parameter. It's implied. |
chipx86
|
|
|
Everything in here is alphabetical but overflow/margin. Also, for margin, use 0 instead of 0px. |
chipx86
|
|
|
Best to use either percentages, or to have a constant defined along with the other constant definitions, so that it's … |
chipx86
|
|
|
A useful JavaScriptism is to do: !!this.options.inTable. That will keep true as true, but turn undefined, null, and false into … |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
The indentation needs to be 4 spaces, not 2. |
chipx86
|
|
|
While here, can you remove the * in this line? |
chipx86
|
|
|
Can you sort these alphabetically by selector? |
chipx86
|
|
|
This needs a doc comment. |
chipx86
|
|
|
Make sure to remove the trailing whitespace, here and below. |
chipx86
|
|
|
Blank line between code and comments. All comments should follow sentence casing, with a trailing period. |
chipx86
|
|
|
This is missing a var. It also should be called $hiddenDiv, since it's a jQuery element reference. Note that you … |
chipx86
|
|
|
This is also missing a var, and needs to be named $link. These same comments all apply to the following … |
chipx86
|
|
|
Best not to hard-code CSS here. Can we use class names instead? Same below where you set display: none. In … |
chipx86
|
|
|
Needs a doc comment. |
chipx86
|
|
|
Space after the comma. |
chipx86
|
|
|
Missing a doc comment. |
chipx86
|
|
|
Best to pull out $(this) into $this, so we don't have to wrap it more than once. |
chipx86
|
|
|
If this shouldn't happen, we need to console.assert it. |
chipx86
|
|
|
I don't believe you need to parseInt this. Can you check whether it comes back as an integer? I believe … |
chipx86
|
|
|
This is really, really wordy, and hard to maintain and review. Please break it up into multiple lines where possible. … |
chipx86
|
|
|
reviewtags and staticfiles are not used. |
chipx86
|
|
|
Can you instead have display: none on issue-container? Also, each button label must be localized. |
chipx86
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Docstrings should be of the form: """Single line summary. Multi-line description. """ |
brennie
|
|
|
Remove trailing whitespace. |
brennie
|
|
|
Method doccomments should use multi-line comment syntax, e.g /* * Hide ... */ |
brennie
|
|
|
Method doccomments should use multi-line comment syntax, e.g /* * Hide ... */ |
brennie
|
|
|
Remove trailing whitespace. |
brennie
|
|
|
Method doccomments should use multi-line comment syntax, e.g /* * Create ... */ |
brennie
|
|
|
Can we assign $(this) to a variable (e.g., $this) and use that? |
brennie
|
|
|
Same here. |
brennie
|
TE
Description: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 2 (+121 -22) |
-
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
-
<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>
-
-
-
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 2) Should begin with a capital
-
-
-
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 2) Use single quotes. Here and elsewhere.
TE
Description: |
|
||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
TE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+121 -21) |
-
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
-
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/
TE
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
TE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+121 -21) |
-
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
TE
Description: |
|
|||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+120 -21) |
-
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
TE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+146 -21) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 6) Col: 1 E302 expected 2 blank lines, found 1
TE
Description: |
|
||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+187 -20) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 7) Col: 1 E302 expected 2 blank lines, found 1
-
-
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.
-
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. """ -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 7) No need for the
Noneparameter. It's implied. -
reviewboard/static/rb/css/pages/reviews.less (Diff revision 7) Everything in here is alphabetical but
overflow/margin.Also, for
margin, use0instead of0px. -
reviewboard/static/rb/css/pages/reviews.less (Diff revision 7) 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.
-
reviewboard/static/rb/js/views/commentIssueBarView.js (Diff revision 7) A useful JavaScriptism is to do:
!!this.options.inTable. That will keeptrueastrue, but turnundefined,null, andfalseintofalse. -
-
reviewboard/static/rb/js/views/commentIssueBarView.js (Diff revision 7) The indentation needs to be 4 spaces, not 2.
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) While here, can you remove the
*in this line? -
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) Can you sort these alphabetically by selector?
-
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) Make sure to remove the trailing whitespace, here and below.
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) Blank line between code and comments.
All comments should follow sentence casing, with a trailing period.
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) This is missing a
var. It also should be called$hiddenDiv, since it's a jQuery element reference.Note that you must have one
varstatement 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. -
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) This is also missing a
var, and needs to be named$link.These same comments all apply to the following function.
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) 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'). -
-
-
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) Best to pull out
$(this)into$this, so we don't have to wrap it more than once. -
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) If this shouldn't happen, we need to
console.assertit. -
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 7) I don't believe you need to
parseIntthis. Can you check whether it comes back as an integer? I believe.date()takes care of this for you. -
reviewboard/templates/reviews/review_issue_summary_table.html (Diff revision 7) 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.
-
reviewboard/templates/reviews/table_comment_issue.html (Diff revision 7) reviewtagsandstaticfilesare not used. -
reviewboard/templates/reviews/table_comment_issue.html (Diff revision 7) Can you instead have
display: noneonissue-container?Also, each button label must be localized.
TE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+203 -19) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 8) Col: 1 E302 expected 2 blank lines, found 1
TE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 9 (+197 -19) |
-
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 -
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 9) Col: 1 E302 expected 2 blank lines, found 1
TE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+198 -19) |
-
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
-
-
reviewboard/reviews/templatetags/reviewtags.py (Diff revision 10) Docstrings should be of the form:
"""Single line summary. Multi-line description. """ -
reviewboard/static/rb/js/views/commentIssueBarView.js (Diff revision 10) Remove trailing whitespace.
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 10) Method doccomments should use multi-line comment syntax, e.g
/* * Hide ... */ -
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 10) Method doccomments should use multi-line comment syntax, e.g
/* * Hide ... */ -
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 10) Remove trailing whitespace.
-
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 10) Method doccomments should use multi-line comment syntax, e.g
/* * Create ... */ -
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 10) Can we assign
$(this)to a variable (e.g.,$this) and use that? -
TE
Summary: |
|
|||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 11 (+207 -19) |
-
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
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