From 9e10b719118333ca20cb5158f6230af47ede7d14 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Mon, 29 Apr 2019 17:43:58 -0400 Subject: [PATCH 01/11] split up the comment threads --- lib/views/reviews-view.js | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index ff83eb86ac..cbeb5b60a2 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -308,7 +308,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 +330,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)} +
+
} ); From 6aa6c4d4b8c56d760697ce63c84104b3c2d05978 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Mon, 29 Apr 2019 17:44:05 -0400 Subject: [PATCH 02/11] add some styling --- styles/review.less | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/styles/review.less b/styles/review.less index f47ff464fb..9bebcf5eb6 100644 --- a/styles/review.less +++ b/styles/review.less @@ -74,6 +74,16 @@ &-section { border-bottom: 1px solid @base-border-color; + + &.resolved-comments { + border-bottom: 0; + padding-left: @component-padding; + .github-Reviews { + &-title, &-header { + color: @text-color-subtle; + } + } + } } &-header { From 84a90a49ace925ecbd6da13c088974fe2b0d6f80 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Mon, 29 Apr 2019 18:16:20 -0400 Subject: [PATCH 03/11] When resolving/unresolving threads, use highlights to hint at users that the comment went under a different list so they are not confused. --- lib/controllers/reviews-controller.js | 18 ++++++++++++++++++ lib/views/reviews-view.js | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/controllers/reviews-controller.js b/lib/controllers/reviews-controller.js index 3c2255d4db..3255271681 100644 --- a/lib/controllers/reviews-controller.js +++ b/lib/controllers/reviews-controller.js @@ -81,6 +81,9 @@ export class BareReviewsController extends React.Component { threadIDsOpen: new Set( this.props.initThreadID ? [this.props.initThreadID] : [], ), + highlightedThreadIDs: new Set( + this.props.initThreadID ? [this.props.initThreadID] : [], + ), }; } @@ -126,6 +129,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 +235,18 @@ 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); + 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 cbeb5b60a2..aef365217a 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -384,7 +384,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.scrollToThreadID === thread.id || this.props.highlightedThreadIDs.has(thread.id); const toggle = evt => { evt.preventDefault(); evt.stopPropagation(); From 748a58a2b741b577c6d4816750c0df3cc42db1e1 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Tue, 30 Apr 2019 12:23:08 -0400 Subject: [PATCH 04/11] consistently use `highlightThread()` to control thread highlighting --- lib/controllers/reviews-controller.js | 9 +++------ lib/views/reviews-view.js | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/controllers/reviews-controller.js b/lib/controllers/reviews-controller.js index 3255271681..2146c5863b 100644 --- a/lib/controllers/reviews-controller.js +++ b/lib/controllers/reviews-controller.js @@ -81,16 +81,14 @@ export class BareReviewsController extends React.Component { threadIDsOpen: new Set( this.props.initThreadID ? [this.props.initThreadID] : [], ), - highlightedThreadIDs: 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); } } @@ -99,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); }); } } diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index aef365217a..30c846c4ef 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -384,7 +384,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 || this.props.highlightedThreadIDs.has(thread.id); + const isHighlighted = this.props.highlightedThreadIDs.has(thread.id); const toggle = evt => { evt.preventDefault(); evt.stopPropagation(); From 9b0e5195573199d8a4a166868e3cfa7a3d48469f Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Wed, 1 May 2019 20:02:07 -0400 Subject: [PATCH 05/11] scroll into view upon resolving/unresolving a thread --- lib/views/reviews-view.js | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 30c846c4ef..10e4295be3 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -507,7 +507,7 @@ export default class ReviewsView extends React.Component { ); @@ -516,7 +516,7 @@ export default class ReviewsView extends React.Component { ); @@ -657,4 +657,23 @@ export default class ReviewsView extends React.Component { return {lineNumber, positionText}; } + + 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); + } + this.scrollToThread(thread.id); + } } From 7eff9bc101d8b28a0caf94fa973df0dc3b6ae058 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Wed, 1 May 2019 20:22:54 -0400 Subject: [PATCH 06/11] remove duplications --- lib/views/reviews-view.js | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 10e4295be3..13f3dbd364 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -110,26 +110,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); } } From 23bedb10b4929b8d9019328e547d3e9acfc030c7 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 2 May 2019 10:05:58 -0400 Subject: [PATCH 07/11] i slept on it and decided that was a bad idea. --- lib/views/reviews-view.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 13f3dbd364..9225c3201d 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -662,6 +662,5 @@ export default class ReviewsView extends React.Component { } else { await this.props.resolveThread(thread); } - this.scrollToThread(thread.id); } } From 1d2b1a3e2e4fc1b4b0a706b1b1db8272c7af864f Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 2 May 2019 12:24:56 -0400 Subject: [PATCH 08/11] add missing proptype --- lib/views/reviews-view.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 9225c3201d..9ce14f033f 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, { From 9f65452a0d0f91ef9ac66707bf9df391860aec29 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 2 May 2019 12:40:19 -0400 Subject: [PATCH 09/11] fix tests --- lib/controllers/reviews-controller.js | 3 +++ test/views/reviews-view.test.js | 1 + 2 files changed, 4 insertions(+) diff --git a/lib/controllers/reviews-controller.js b/lib/controllers/reviews-controller.js index 2146c5863b..483a9ffd0a 100644 --- a/lib/controllers/reviews-controller.js +++ b/lib/controllers/reviews-controller.js @@ -239,6 +239,9 @@ export class BareReviewsController extends React.Component { }, () => { setTimeout(() => this.setState(state => { state.highlightedThreadIDs.delete(threadID); + if (state.scrollToThreadID === threadID) { + return {scrollToThreadID: null}; + } return {}; }), FLASH_DELAY); }); diff --git a/test/views/reviews-view.test.js b/test/views/reviews-view.test.js index 74bd870fbc..066fc1435c 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', From cc3d92566674ba423622fc5bb13c5b6bddca9ef9 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 2 May 2019 13:18:14 -0400 Subject: [PATCH 10/11] styling --- styles/review.less | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/styles/review.less b/styles/review.less index 9bebcf5eb6..2911a708cd 100644 --- a/styles/review.less +++ b/styles/review.less @@ -79,9 +79,13 @@ border-bottom: 0; padding-left: @component-padding; .github-Reviews { - &-title, &-header { + &-title { color: @text-color-subtle; } + &-header { + color: @text-color-subtle; + padding-top: @component-padding * 0.5; + } } } } From 200dcc2231366828e5a466ea64a5a8bae7b60464 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Mon, 6 May 2019 15:09:46 -0400 Subject: [PATCH 11/11] add/modify some tests --- lib/controllers/reviews-controller.js | 2 +- lib/views/reviews-view.js | 1 + test/controllers/reviews-controller.test.js | 8 ++++++-- test/views/reviews-view.test.js | 7 +++++++ 4 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/controllers/reviews-controller.js b/lib/controllers/reviews-controller.js index 483a9ffd0a..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 { diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 9ce14f033f..e244abb1ad 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -649,6 +649,7 @@ export default class ReviewsView extends React.Component { return {lineNumber, positionText}; } + /* istanbul ignore next */ scrollToThread(threadID) { const threadHolder = this.threadHolders.get(threadID); if (threadHolder) { 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 066fc1435c..f5513ee9de 100644 --- a/test/views/reviews-view.test.js +++ b/test/views/reviews-view.test.js @@ -292,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);