Merge pull request #2117 from atom/vy/review-comments-grouping

Group resolved comments

Author: vanessayuenn

lib/controllers/reviews-controller.js

@@ -14,7 +14,7 @@ import unresolveReviewThreadMutation from '../mutations/unresolve-review-thread'
 import IssueishDetailItem from '../items/issueish-detail-item';
 import {addEvent} from '../reporter-proxy';
 
-// Milliseconds to leave scrollToThreadID non-null before reverting.
+// Milliseconds to update highlightedThreadIDs
 const FLASH_DELAY = 1500;
 
 export class BareReviewsController extends React.Component {
@@ -81,13 +81,14 @@ export class BareReviewsController extends React.Component {
       threadIDsOpen: new Set(
         this.props.initThreadID ? [this.props.initThreadID] : [],
       ),
+      highlightedThreadIDs: new Set(),
     };
   }
 
   componentDidMount() {
     const {scrollToThreadID} = this.state;
     if (scrollToThreadID) {
-      setTimeout(() => this.setState({scrollToThreadID: null}), FLASH_DELAY);
+      this.highlightThread(scrollToThreadID);
     }
   }
 
@@ -96,9 +97,8 @@ export class BareReviewsController extends React.Component {
     if (initThreadID && initThreadID !== prevProps.initThreadID) {
       this.setState(prev => {
         prev.threadIDsOpen.add(initThreadID);
+        this.highlightThread(initThreadID);
         return {commentSectionOpen: true, scrollToThreadID: initThreadID};
-      }, () => {
-        setTimeout(() => this.setState({scrollToThreadID: null}), FLASH_DELAY);
       });
     }
   }
@@ -126,6 +126,7 @@ export class BareReviewsController extends React.Component {
             summarySectionOpen={this.state.summarySectionOpen}
             commentSectionOpen={this.state.commentSectionOpen}
             threadIDsOpen={this.state.threadIDsOpen}
+            highlightedThreadIDs={this.state.highlightedThreadIDs}
             scrollToThreadID={this.state.scrollToThreadID}
 
             moreContext={this.moreContext}
@@ -231,6 +232,21 @@ export class BareReviewsController extends React.Component {
     return {};
   }, resolve));
 
+  highlightThread = threadID => {
+    this.setState(state => {
+      state.highlightedThreadIDs.add(threadID);
+      return {};
+    }, () => {
+      setTimeout(() => this.setState(state => {
+        state.highlightedThreadIDs.delete(threadID);
+        if (state.scrollToThreadID === threadID) {
+          return {scrollToThreadID: null};
+        }
+        return {};
+      }), FLASH_DELAY);
+    });
+  }
+
   resolveThread = async thread => {
     if (thread.viewerCanResolve) {
       // optimistically hide the thread to avoid jankiness;
@@ -242,6 +258,7 @@ export class BareReviewsController extends React.Component {
           viewerID: this.props.viewer.id,
           viewerLogin: this.props.viewer.login,
         });
+        this.highlightThread(thread.id);
         addEvent('resolve-comment-thread', {package: 'github'});
       } catch (err) {
         this.showThreadID(thread.id);
@@ -258,6 +275,7 @@ export class BareReviewsController extends React.Component {
           viewerID: this.props.viewer.id,
           viewerLogin: this.props.viewer.login,
         });
+        this.highlightThread(thread.id);
         addEvent('unresolve-comment-thread', {package: 'github'});
       } catch (err) {
         this.props.reportMutationErrors('Unable to unresolve the comment thread', err);

lib/views/reviews-view.js

@@ -54,6 +54,9 @@ export default class ReviewsView extends React.Component {
     threadIDsOpen: PropTypes.shape({
       has: PropTypes.func.isRequired,
     }),
+    highlightedThreadIDs: PropTypes.shape({
+      has: PropTypes.func.isRequired,
+    }),
     postingToThreadID: PropTypes.string,
     scrollToThreadID: PropTypes.string,
     // Structure: Map< relativePath: String, {
@@ -110,26 +113,14 @@ export default class ReviewsView extends React.Component {
   componentDidMount() {
     const {scrollToThreadID} = this.props;
     if (scrollToThreadID) {
-      const threadHolder = this.threadHolders.get(scrollToThreadID);
-      if (threadHolder) {
-        threadHolder.map(element => {
-          element.scrollIntoViewIfNeeded();
-          return null; // shh, eslint
-        });
-      }
+      this.scrollToThread(scrollToThreadID);
     }
   }
 
   componentDidUpdate(prevProps) {
     const {scrollToThreadID} = this.props;
     if (scrollToThreadID && scrollToThreadID !== prevProps.scrollToThreadID) {
-      const threadHolder = this.threadHolders.get(scrollToThreadID);
-      if (threadHolder) {
-        threadHolder.map(element => {
-          element.scrollIntoViewIfNeeded();
-          return null; // shh, eslint
-        });
-      }
+      this.scrollToThread(scrollToThreadID);
     }
   }
 
@@ -309,7 +300,8 @@ export default class ReviewsView extends React.Component {
       return null;
     }
 
-    const resolvedThreads = commentThreads.filter(pair => pair.thread.isResolved).length;
+    const resolvedThreads = commentThreads.filter(pair => pair.thread.isResolved);
+    const unresolvedThreads = commentThreads.filter(pair => !pair.thread.isResolved);
 
     const toggleComments = evt => {
       evt.preventDefault();
@@ -330,16 +322,28 @@ export default class ReviewsView extends React.Component {
           <span className="github-Reviews-progress">
             <span className="github-Reviews-count">
               Resolved
-              {' '}<span className="github-Reviews-countNr">{resolvedThreads}</span>{' '}
+              {' '}<span className="github-Reviews-countNr">{resolvedThreads.length}</span>{' '}
               of
-              {' '}<span className="github-Reviews-countNr">{commentThreads.length}</span>
+              {' '}<span className="github-Reviews-countNr">{resolvedThreads.length + unresolvedThreads.length}</span>
             </span>
-            <progress className="github-Reviews-progessBar" value={resolvedThreads} max={commentThreads.length} />
+            <progress
+              className="github-Reviews-progessBar" value={resolvedThreads.length}
+              max={resolvedThreads.length + unresolvedThreads.length}
+            />
           </span>
         </summary>
-        <main className="github-Reviews-container">
-          {commentThreads.map(this.renderReviewCommentThread)}
-        </main>
+
+        {unresolvedThreads.length > 0 && <main className="github-Reviews-container">
+          {unresolvedThreads.map(this.renderReviewCommentThread)}
+        </main>}
+        {resolvedThreads.length > 0 && <details className="github-Reviews-section resolved-comments" open>
+          <summary className="github-Reviews-header">
+            <span className="github-Reviews-title">Resolved</span>
+          </summary>
+          <main className="github-Reviews-container">
+            {resolvedThreads.map(this.renderReviewCommentThread)}
+          </main>
+        </details>}
 
       </details>
     );
@@ -372,7 +376,7 @@ export default class ReviewsView extends React.Component {
     const openDiffClasses = cx('icon-diff', ...navButtonClasses);
 
     const isOpen = this.props.threadIDsOpen.has(thread.id);
-    const isHighlighted = this.props.scrollToThreadID === thread.id;
+    const isHighlighted = this.props.highlightedThreadIDs.has(thread.id);
     const toggle = evt => {
       evt.preventDefault();
       evt.stopPropagation();
@@ -495,7 +499,7 @@ export default class ReviewsView extends React.Component {
         <button
           className="github-Review-resolveButton btn icon icon-check"
           title="Unresolve conversation"
-          onClick={() => this.props.unresolveThread(thread)}>
+          onClick={() => this.resolveUnresolveThread(thread)}>
           Unresolve conversation
         </button>
       );
@@ -504,7 +508,7 @@ export default class ReviewsView extends React.Component {
         <button
           className="github-Review-resolveButton btn icon icon-check"
           title="Resolve conversation"
-          onClick={() => this.props.resolveThread(thread)}>
+          onClick={() => this.resolveUnresolveThread(thread)}>
           Resolve conversation
         </button>
       );
@@ -646,4 +650,23 @@ export default class ReviewsView extends React.Component {
 
     return {lineNumber, positionText};
   }
+
+  /* istanbul ignore next */
+  scrollToThread(threadID) {
+    const threadHolder = this.threadHolders.get(threadID);
+    if (threadHolder) {
+      threadHolder.map(element => {
+        element.scrollIntoViewIfNeeded();
+        return null; // shh, eslint
+      });
+    }
+  }
+
+  async resolveUnresolveThread(thread) {
+    if (thread.isResolved) {
+      await this.props.unresolveThread(thread);
+    } else {
+      await this.props.resolveThread(thread);
+    }
+  }
 }

styles/review.less

@@ -74,6 +74,20 @@
 
   &-section {
     border-bottom: 1px solid @base-border-color;
+
+    &.resolved-comments {
+      border-bottom: 0;
+      padding-left: @component-padding;
+      .github-Reviews {
+        &-title {
+          color: @text-color-subtle;
+        }
+        &-header {
+          color: @text-color-subtle;
+          padding-top: @component-padding * 0.5;
+        }
+      }
+    }
   }
 
   &-header {

test/controllers/reviews-controller.test.js

@@ -95,20 +95,22 @@ describe('ReviewsController', function() {
     assert.strictEqual(opWrapper.find(ReviewsView).prop('extra'), extra);
   });
 
-  it('scrolls to a specific thread on mount', function() {
+  it('scrolls to and highlight a specific thread on mount', function() {
     clock = sinon.useFakeTimers();
     const wrapper = shallow(buildApp({initThreadID: 'thread0'}));
     let opWrapper = wrapper.find(PullRequestCheckoutController).renderProp('children')(noop);
 
     assert.include(opWrapper.find(ReviewsView).prop('threadIDsOpen'), 'thread0');
+    assert.include(opWrapper.find(ReviewsView).prop('highlightedThreadIDs'), 'thread0');
     assert.strictEqual(opWrapper.find(ReviewsView).prop('scrollToThreadID'), 'thread0');
 
     clock.tick(2000);
     opWrapper = wrapper.find(PullRequestCheckoutController).renderProp('children')(noop);
+    assert.notInclude(opWrapper.find(ReviewsView).prop('highlightedThreadIDs'), 'thread0');
     assert.isNull(opWrapper.find(ReviewsView).prop('scrollToThreadID'));
   });
 
-  it('scrolls to a specific thread on update', function() {
+  it('scrolls to and highlight a specific thread on update', function() {
     clock = sinon.useFakeTimers();
     const wrapper = shallow(buildApp());
     let opWrapper = wrapper.find(PullRequestCheckoutController).renderProp('children')(noop);
@@ -118,11 +120,13 @@ describe('ReviewsController', function() {
     opWrapper = wrapper.find(PullRequestCheckoutController).renderProp('children')(noop);
 
     assert.include(opWrapper.find(ReviewsView).prop('threadIDsOpen'), 'hang-by-a-thread');
+    assert.include(opWrapper.find(ReviewsView).prop('highlightedThreadIDs'), 'hang-by-a-thread');
     assert.isTrue(opWrapper.find(ReviewsView).prop('commentSectionOpen'));
     assert.strictEqual(opWrapper.find(ReviewsView).prop('scrollToThreadID'), 'hang-by-a-thread');
 
     clock.tick(2000);
     opWrapper = wrapper.find(PullRequestCheckoutController).renderProp('children')(noop);
+    assert.notInclude(opWrapper.find(ReviewsView).prop('highlightedThreadIDs'), 'hang-by-a-thread');
     assert.isNull(opWrapper.find(ReviewsView).prop('scrollToThreadID'));
   });
 

test/views/reviews-view.test.js

@@ -36,6 +36,7 @@ describe('ReviewsView', function() {
       summarySectionOpen: true,
       commentSectionOpen: true,
       threadIDsOpen: new Set(),
+      highlightedThreadIDs: new Set(),
 
       number: 100,
       repo: 'github',
@@ -318,6 +319,13 @@ describe('ReviewsView', function() {
       assert.strictEqual(comment.find('em').text(), 'This comment was hidden');
     });
 
+    it('groups comments by their resolved status', function() {
+      const unresolvedThreads = wrapper.find('.github-Reviews-section.comments').find('.github-Review');
+      const resolvedThreads = wrapper.find('.github-Reviews-section.resolved-comments').find('.github-Review');
+      assert.lengthOf(resolvedThreads, 1);
+      assert.lengthOf(unresolvedThreads, 3);
+    });
+
     describe('indication that a comment has been edited', function() {
       it('indicates that a review summary comment has been edited', function() {
         const summary = wrapper.find('.github-ReviewSummary').at(0);