Summary

Make descriptions in the issue summary table expandable so full comments are viewable in table on click

Review Request #6910 — Created Feb. 5, 2015 and discarded March 13, 2015, 6:05 p.m.

Information

Owner

terfan

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Added a new div in review_issue_summary_table.html that contains full comment text and is initially hidden.
Added a new CSS class, 'issue-summary-collapsed,' to be applied to this hidden div when the summary is collapsed.
Added two new JS methods, onPreviewClicked and onFullClicked, for handling user clicks.

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.

Testing Done

Manual testing:
- 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.

Files

Issues

Description	From	Last Updated
There should only be one var statement at the top of the function.	brennie	Feb. 28, 2015, 6:23 a.m.
Space after comma, I believe. :)	SU Sunxperous	Feb. 26, 2015, 6:25 a.m.
Any particular reason you're not just using .toggle()?	brennie	Feb. 28, 2015, 9:05 a.m.
Space after ,	SU Sunxperous	Feb. 26, 2015, 6:25 a.m.
And space after comma, and the next line (151) as well.	SU Sunxperous	Feb. 26, 2015, 6:25 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/issueSummaryTableView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/views/issueSummaryTableView.js

Hey Teresa - would you mind attaching some screenshots to show us what this looks like in practice?

terfan Feb. 5, 2015, 8:07 a.m.

Sure, although screenshots won't capture the animation--should I also try to screencast a video?

mike_conley Feb. 5, 2015, 8:13 a.m.

Oh, yes please, if it's not too much trouble.

Added Files:

Added Files:

<p>Instead of having a separate hidden div, can this be done using css?</p>

<div class="codehilite"><pre>width: 100%;

white-space: nowrap;

overflow: hidden;

text-overflow: ellipsis;

</pre></div>

terfan Feb. 13, 2015, 1:27 p.m.

Thanks for the suggestion! I tried your idea but I think it actually complicates it more, because you can't just set the width of the cell to be 100% or any specific value, as you want it to change the table width dynamically with any window resizing.

wudi Feb. 13, 2015, 2:14 p.m.

I think you can use an outer div to workaround the cell width, something like this:

div.issue-comment-wrapper {
    position: relative;
    div.issue-preview {
        width: 100%;
        white-space: nowrap;
        overflow: hidden;
        text-overflow: ellipsis;
        position: absolute;
    }
}


The width of the outer div will be adjusted dynamically with window sizing.

terfan Feb. 14, 2015, 9:32 p.m.

In this case though, if I just take away the white-space nowrap when I want the full text displayed, the cell doesn't expand to fit the rest of the text, it just overflows into the next rows.

wudi Feb. 15, 2015, 12:42 a.m.

That's due to the position absolute property. You do not need the properties under .issue-preview when you want the full text displayed, so you can remove the class from the div when you click to expand.

terfan Feb. 15, 2015, 12:25 p.m.

Ah, got it to work, although with this approach the ellipsis can't be made into a link that can be used to expand as David suggested below. But if we decide on something else, I will consider your idea!

Hmm, just a question about the interaction here. How will people get to the actual review comment?

terfan Feb. 13, 2015, 5:06 p.m.

The next steps in the project (as was outlined by Christian) are to add buttons for resolving and reopening issues, as well as the number of replies. When you click the number of replies it will take you to the actual comment. But that's a good point, perhaps I should just mark this review as a WIP and wait until I have added the other features to submit it for real consideration?

Other suggestions for how to handle it are welcome. I might try to make some mock-ups of different options.

david Feb. 13, 2015, 5:14 p.m.

Perhaps we can make the ellipsis a link, and use that to expand, while clicking on the row keeps the current behavior? I feel like jumping to the comment to see context and be able to reply is the most important part of the table.

terfan Feb. 15, 2015, 12:25 p.m.

Not a bad idea, although I'm not sure how discoverable the expand feature will be if we go with that. What about a link to the comment once the cell is expanded? (see attached photo, 'alternative_option')

david Feb. 16, 2015, 11:05 a.m.

That would work, though I'd suggest always showing that link (not just when the cell is expanded).

terfan Feb. 19, 2015, 9:10 a.m.

Updated. Let me know what you think (see expandedv2 image)

david Feb. 19, 2015, 4:26 p.m.

What does it look like before you expand?

terfan Feb. 19, 2015, 8:38 p.m.

The first and third comments are not expanded (only the middle one is).

david Feb. 20, 2015, 12:58 p.m.

Can we put "See comment" on the same row as the text when not expanded? This will make each line twice the current height.

terfan Feb. 26, 2015, 9:36 a.m.

Done. (see updated images collapsed.png and expanded.png)

david Feb. 26, 2015, 1:45 p.m.
```
I like that a lot!
```

Added Files:

Added Files:

reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Space after comma, I believe. :)
```
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Space after ,
```
reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
And space after comma, and the next line (151) as well.
```

Added Files:

reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 1)

The issue has been resolved. Show all issues

There should only be one var statement at the top of the function.

reviewboard/static/rb/js/views/issueSummaryTableView.js (Diff revision 1)

The issue has been resolved. Show all issues

Any particular reason you're not just using .toggle()?

TE

terfan Feb. 28, 2015, 9:05 a.m.
```
Nope! Thanks for the suggestion.
```

Teresa,

It looks like you marked Barret's issues as 'fixed' but haven't yet uploaded a new revision. When you get a moment, can you do so? (I know it's spring break, so no huge rush)

terfan March 13, 2015, 3:44 p.m.

I accidentally uploaded the changes as a whole new review request here last week (https://reviews.reviewboard.org/r/6997/). My mistake! Is there a way I can combine the two?

david March 13, 2015, 3:49 p.m.

There's no way to combine them. I'd just choose one to move forward on (maybe the new one), and then edit the description to link to the other. Whichever one is obsolete should be closed as "Discarded".

Status:: Discarded