diff --git a/lib/controllers/reviews-controller.js b/lib/controllers/reviews-controller.js index 3c2255d4db..877458382e 100644 --- a/lib/controllers/reviews-controller.js +++ b/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); diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index ff83eb86ac..e244abb1ad 100644 --- a/lib/views/reviews-view.js +++ b/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); } } @@ -308,7 +299,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(); @@ -329,16 +321,28 @@ export default class ReviewsView extends React.Component { Resolved - {' '}{resolvedThreads}{' '} + {' '}{resolvedThreads.length}{' '} of - {' '}{commentThreads.length} + {' '}{resolvedThreads.length + unresolvedThreads.length} - + -
- {commentThreads.map(this.renderReviewCommentThread)} -
+ + {unresolvedThreads.length > 0 &&
+ {unresolvedThreads.map(this.renderReviewCommentThread)} +
} + {resolvedThreads.length > 0 &&
+ + Resolved + +
+ {resolvedThreads.map(this.renderReviewCommentThread)} +
+
} ); @@ -371,7 +375,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(); @@ -494,7 +498,7 @@ export default class ReviewsView extends React.Component { ); @@ -503,7 +507,7 @@ export default class ReviewsView extends React.Component { ); @@ -644,4 +648,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); + } + } } diff --git a/styles/review.less b/styles/review.less index f47ff464fb..2911a708cd 100644 --- a/styles/review.less +++ b/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 { diff --git a/test/controllers/reviews-controller.test.js b/test/controllers/reviews-controller.test.js index 35f6c90b63..471e6ec3aa 100644 --- a/test/controllers/reviews-controller.test.js +++ b/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')); }); diff --git a/test/views/reviews-view.test.js b/test/views/reviews-view.test.js index 74bd870fbc..f5513ee9de 100644 --- a/test/views/reviews-view.test.js +++ b/test/views/reviews-view.test.js @@ -35,6 +35,7 @@ describe('ReviewsView', function() { summarySectionOpen: true, commentSectionOpen: true, threadIDsOpen: new Set(), + highlightedThreadIDs: new Set(), number: 100, repo: 'github', @@ -291,6 +292,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);