Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Group resolved comments #2117

Merged
merged 11 commits into from
May 7, 2019
Merged

Group resolved comments #2117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9e10b71
split up the comment threads
vanessayuenn Apr 29, 2019
6aa6c4d
add some styling
vanessayuenn Apr 29, 2019
84a90a4
When resolving/unresolving threads, use highlights to hint at users t…
vanessayuenn Apr 29, 2019
748a58a
consistently use `highlightThread()` to control thread highlighting
vanessayuenn Apr 30, 2019
9b0e519
scroll into view upon resolving/unresolving a thread
vanessayuenn May 2, 2019
7eff9bc
remove duplications
vanessayuenn May 2, 2019
23bedb1
i slept on it and decided that was a bad idea.
vanessayuenn May 2, 2019
1d2b1a3
add missing proptype
vanessayuenn May 2, 2019
9f65452
fix tests
vanessayuenn May 2, 2019
cc3d925
styling
vanessayuenn May 2, 2019
200dcc2
add/modify some tests
vanessayuenn May 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions lib/controllers/reviews-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we had a way to collapse the setState updater functions from here and highlightThread into a single, atomic transaction. I'm pretty sure that's how it's working anyway and this is much cleaner but still 😛

});
}
}
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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);
Expand Down
71 changes: 47 additions & 24 deletions lib/views/reviews-view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

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);
}
}

Expand Down Expand Up @@ -308,7 +299,8 @@ export default class ReviewsView extends React.Component {
return null;
}

const resolvedThreads = commentThreads.filter(pair => pair.thread.isResolved).length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, should have called this resolvedThreadCount or something before. 👍

const resolvedThreads = commentThreads.filter(pair => pair.thread.isResolved);
const unresolvedThreads = commentThreads.filter(pair => !pair.thread.isResolved);

const toggleComments = evt => {
evt.preventDefault();
Expand All @@ -329,16 +321,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>
);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -494,7 +498,7 @@ export default class ReviewsView extends React.Component {
<button
className="github-Review-resolveButton btn btn-primary icon icon-check"
title="Unresolve conversation"
onClick={() => this.props.unresolveThread(thread)}>
onClick={() => this.resolveUnresolveThread(thread)}>
Unresolve conversation
</button>
);
Expand All @@ -503,7 +507,7 @@ export default class ReviewsView extends React.Component {
<button
className="github-Review-resolveButton btn btn-primary icon icon-check"
title="Resolve conversation"
onClick={() => this.props.resolveThread(thread)}>
onClick={() => this.resolveUnresolveThread(thread)}>
Resolve conversation
</button>
);
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really need to make a RefHolder::forEach method to make these cases nicer...

});
}
}

async resolveUnresolveThread(thread) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We keep hitting situations where we need to name methods that toggle something without having a good word for the toggling action. There isn't really a good alternative for this:

  • resolveOrUnresolveThread()
  • toggleThreadResolution()
  • ... ?

if (thread.isResolved) {
await this.props.unresolveThread(thread);
} else {
await this.props.resolveThread(thread);
}
}
}
14 changes: 14 additions & 0 deletions styles/review.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 6 additions & 2 deletions test/controllers/reviews-controller.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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'));
});

Expand Down
8 changes: 8 additions & 0 deletions test/views/reviews-view.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ describe('ReviewsView', function() {
summarySectionOpen: true,
commentSectionOpen: true,
threadIDsOpen: new Set(),
highlightedThreadIDs: new Set(),

number: 100,
repo: 'github',
Expand Down Expand Up @@ -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);
Expand Down