From d1cf331cf5bc23aec91b8521749dfeef624d4475 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 09:32:49 -0500 Subject: [PATCH 001/738] Stub out "Reviews" component tree --- lib/containers/reviews-container.js | 7 +++++++ lib/controllers/reviews-controller.js | 7 +++++++ lib/items/reviews-item.js | 7 +++++++ lib/views/reviews-view.js | 7 +++++++ 4 files changed, 28 insertions(+) create mode 100644 lib/containers/reviews-container.js create mode 100644 lib/controllers/reviews-controller.js create mode 100644 lib/items/reviews-item.js create mode 100644 lib/views/reviews-view.js diff --git a/lib/containers/reviews-container.js b/lib/containers/reviews-container.js new file mode 100644 index 0000000000..8a531f7521 --- /dev/null +++ b/lib/containers/reviews-container.js @@ -0,0 +1,7 @@ +import React from 'react'; + +export default class ReviewsContainer extends React.Component { + render() { + return null; + } +} diff --git a/lib/controllers/reviews-controller.js b/lib/controllers/reviews-controller.js new file mode 100644 index 0000000000..cfac131b33 --- /dev/null +++ b/lib/controllers/reviews-controller.js @@ -0,0 +1,7 @@ +import React from 'react'; + +export default class ReviewsController extends React.Component { + render() { + return null; + } +} diff --git a/lib/items/reviews-item.js b/lib/items/reviews-item.js new file mode 100644 index 0000000000..83d44c4fcc --- /dev/null +++ b/lib/items/reviews-item.js @@ -0,0 +1,7 @@ +import React from 'react'; + +export default class ReviewsItem extends React.Component { + render() { + return null; + } +} diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js new file mode 100644 index 0000000000..4e92166fae --- /dev/null +++ b/lib/views/reviews-view.js @@ -0,0 +1,7 @@ +import React from 'react'; + +export default class ReviewsView extends React.Component { + render() { + return null; + } +} From 2840bd44c86a191e030783509730cf9a8bcdd8aa Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 10:16:19 -0500 Subject: [PATCH 002/738] Item boilerplate for ReviewsItem --- lib/items/reviews-item.js | 76 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/lib/items/reviews-item.js b/lib/items/reviews-item.js index 83d44c4fcc..17c85428cb 100644 --- a/lib/items/reviews-item.js +++ b/lib/items/reviews-item.js @@ -1,7 +1,81 @@ import React from 'react'; +import PropTypes from 'prop-types'; +import {Emitter} from 'event-kit'; + +import {GithubLoginModelPropType, WorkdirContextPoolPropType} from '../prop-types'; +import ReviewsContainer from '../containers/reviews-container'; export default class ReviewsItem extends React.Component { + static propTypes = { + // Parsed from URI + host: PropTypes.string.isRequired, + owner: PropTypes.string.isRequired, + repo: PropTypes.string.isRequired, + number: PropTypes.number.isRequired, + workdir: PropTypes.string.isRequired, + + // Package models + workdirContextPool: WorkdirContextPoolPropType.isRequired, + loginModel: GithubLoginModelPropType.isRequired, + } + + static uriPattern = 'atom-github://reviews/{host}/{owner}/{repo}/{number}?workdir={workdir}' + + static buildURI(host, owner, repo, number, workdir) { + return 'atom-github://reviews/' + + encodeURIComponent(host) + '/' + + encodeURIComponent(owner) + '/' + + encodeURIComponent(repo) + '/' + + encodeURIComponent(number) + '?workdir=' + encodeURIComponent(workdir); + } + + constructor(props) { + super(props); + + this.emitter = new Emitter(); + this.isDestroyed = false; + } + render() { - return null; + return ( + + ); + } + + getTitle() { + return `Reviews #${this.props.number}`; + } + + getDefaultLocation() { + return 'right'; + } + + getPreferredWidth() { + return 400; + } + + destroy() { + /* istanbul ignore else */ + if (!this.isDestroyed) { + this.emitter.emit('did-destroy'); + this.isDestroyed = true; + } + } + + onDidDestroy(callback) { + return this.emitter.on('did-destroy', callback); + } + + serialize() { + return { + deserializer: 'ReviewsStub', + uri: this.constructor.buildURI( + this.props.host, + this.props.owner, + this.props.repo, + this.props.number, + this.props.workdir, + ), + }; } } From 66658d7e63d73886e9c84f62ea758597c1b54ab1 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 10:57:06 -0500 Subject: [PATCH 003/738] ReviewsContainer boilerplate: API token, repository data, and GraphQL --- lib/containers/reviews-container.js | 142 +++++++++++++++++++++++++++- 1 file changed, 141 insertions(+), 1 deletion(-) diff --git a/lib/containers/reviews-container.js b/lib/containers/reviews-container.js index 8a531f7521..7de75d094e 100644 --- a/lib/containers/reviews-container.js +++ b/lib/containers/reviews-container.js @@ -1,7 +1,147 @@ import React from 'react'; +import PropTypes from 'prop-types'; +import yubikiri from 'yubikiri'; +import {QueryRenderer, graphql} from 'react-relay'; + +import {PAGE_SIZE} from '../helpers'; +import {GithubLoginModelPropType, EndpointPropType} from '../prop-types'; +import {UNAUTHENTICATED, INSUFFICIENT} from '../shared/keytar-strategy'; +import ObserveModel from '../views/observe-model'; +import LoadingView from '../views/loading-view'; +import GithubLoginView from '../views/github-login-view'; +import QueryErrorView from '../views/query-error-view'; +import RelayNetworkLayerManager from '../relay-network-layer-manager'; +import RelayEnvironment from '../views/relay-environment'; +import ReviewsController from '../controllers/reviews-controller'; export default class ReviewsContainer extends React.Component { + static propTypes = { + // Connection + endpoint: EndpointPropType.isRequired, + + // Pull request selection criteria + owner: PropTypes.string.isRequired, + repo: PropTypes.string.isRequired, + number: PropTypes.number.isRequired, + + // Package models + repository: PropTypes.object.isRequired, + loginModel: GithubLoginModelPropType.isRequired, + } + render() { - return null; + return ( + + {tokenData => this.renderWithToken(tokenData)} + + ); + } + + renderWithToken(tokenData) { + if (!tokenData) { + return ; + } + + if (tokenData.token === UNAUTHENTICATED) { + return ; + } + + if (tokenData.token === INSUFFICIENT) { + return ( + +

+ Your token no longer has sufficient authorizations. Please re-authenticate and generate a new one. +

+ + ); + } + + return ( + + {repoData => this.renderWithRepositoryData(repoData, tokenData.token)} + + ); } + + renderWithRepositoryData(repoData, token) { + if (!repoData) { + return ; + } + + const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.endpoint, token); + const query = graphql` + query reviewsContainerQuery + ( + $repoOwner: String! + $repoName: String! + $prNumber: Int! + $reviewCount: Int! + $reviewCursor: String + $commentCount: Int! + $commentCursor: String + ) { + repository(owner: $repoOwner, name: $repoName) { + id + } + } + `; + const variables = { + repoOwner: this.props.owner, + repoName: this.props.repo, + prNumber: this.props.number, + reviewCount: PAGE_SIZE, + reviewCursor: null, + commentCount: PAGE_SIZE, + commentCursor: null, + }; + + return ( + + this.renderWithResult(queryResult, repoData)} + /> + + ); + } + + renderWithResult({error, props, retry}, repoData) { + if (error) { + return ( + + ); + } + + if (!props) { + return ; + } + + return ( + + ); + } + + fetchToken = loginModel => loginModel.getToken(this.props.endpoint.getLoginAccount()); + + fetchRepositoryData = repository => { + return yubikiri({ + branches: repository.getBranches(), + isLoading: repository.isLoading(), + isPresent: repository.isPresent(), + }); + } + + handleLogin = token => this.props.loginModel.setToken(this.props.endpoint.getLoginAccount(), token); + + handleLogout = this.props.loginModel.removeToken(this.props.endpoint.getLoginAccount()); } From d82a26893c1dc258fad8cf68230c62e2c7b6d7c6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 10:57:25 -0500 Subject: [PATCH 004/738] Derive and pass repository and endpoint in ReviewsItem --- lib/items/reviews-item.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/items/reviews-item.js b/lib/items/reviews-item.js index 17c85428cb..f8fbb0df01 100644 --- a/lib/items/reviews-item.js +++ b/lib/items/reviews-item.js @@ -3,6 +3,7 @@ import PropTypes from 'prop-types'; import {Emitter} from 'event-kit'; import {GithubLoginModelPropType, WorkdirContextPoolPropType} from '../prop-types'; +import {getEndpoint} from '../models/endpoint'; import ReviewsContainer from '../containers/reviews-container'; export default class ReviewsItem extends React.Component { @@ -37,8 +38,15 @@ export default class ReviewsItem extends React.Component { } render() { + const endpoint = getEndpoint(this.props.host); + const repository = this.props.workdirContextPool.add(this.props.workdir).getRepository(); + return ( - + ); } From df61f33237104c69042b82276cdd22f37174ac9f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 10:57:36 -0500 Subject: [PATCH 005/738] Render ReviewsView from ReviewsController --- lib/controllers/reviews-controller.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/controllers/reviews-controller.js b/lib/controllers/reviews-controller.js index cfac131b33..e586fda16f 100644 --- a/lib/controllers/reviews-controller.js +++ b/lib/controllers/reviews-controller.js @@ -1,7 +1,13 @@ import React from 'react'; +import ReviewsView from '../views/reviews-view'; + export default class ReviewsController extends React.Component { render() { - return null; + return ( + + ); } } From 44f214aa0bc4bc4a72569711a58695ada739aac8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 10:57:44 -0500 Subject: [PATCH 006/738] Placeholder ReviewsView --- lib/views/reviews-view.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 4e92166fae..ce8ec6b45d 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -1,7 +1,13 @@ import React from 'react'; +import {inspect} from 'util'; export default class ReviewsView extends React.Component { render() { - return null; + return ( +
+

HELL YEAH PR REVIEW COMMENTS

+ 
{inspect(this.props, {depth: 4})}
+
+ ); } } From c6926e35bf67395cfd8fbfb3a4b70b061c2a2134 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 11:01:21 -0500 Subject: [PATCH 007/738] Register ReviewsItem opener in RootController --- lib/controllers/root-controller.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/controllers/root-controller.js b/lib/controllers/root-controller.js index b911f11b3e..e83f80ad6b 100644 --- a/lib/controllers/root-controller.js +++ b/lib/controllers/root-controller.js @@ -22,6 +22,7 @@ import CommitDetailItem from '../items/commit-detail-item'; import CommitPreviewItem from '../items/commit-preview-item'; import GitTabItem from '../items/git-tab-item'; import GitHubTabItem from '../items/github-tab-item'; +import ReviewsItem from '../items/reviews-item'; import StatusBarTileController from './status-bar-tile-controller'; import RepositoryConflictController from './repository-conflict-controller'; import GitCacheView from '../views/git-cache-view'; @@ -425,6 +426,22 @@ export default class RootController extends React.Component { /> )} + + {({itemHolder, params}) => ( + + )} + {({itemHolder}) => } From c68452a04027d821b3ee6818d94d1dcaddbbeb7d Mon Sep 17 00:00:00 2001 From: annthurium Date: Fri, 1 Mar 2019 08:38:20 -0800 Subject: [PATCH 008/738] add `ReviewsFooterView` component --- lib/views/pr-detail-view.js | 8 ++----- lib/views/reviews-footer-view.js | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 lib/views/reviews-footer-view.js diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 92d660b024..794842b55a 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -15,6 +15,7 @@ import EmojiReactionsView from '../views/emoji-reactions-view'; import IssueishBadge from '../views/issueish-badge'; import PrCommitsView from '../views/pr-commits-view'; import PrStatusesView from '../views/pr-statuses-view'; +import ReviewsFooterView from '../views/reviews-footer-view'; import {PAGE_SIZE} from '../helpers'; class CheckoutState { @@ -275,12 +276,7 @@ export class BarePullRequestDetailView extends React.Component { {this.renderPullRequestBody(pullRequest)} - - + ); diff --git a/lib/views/reviews-footer-view.js b/lib/views/reviews-footer-view.js new file mode 100644 index 0000000000..1e444f2943 --- /dev/null +++ b/lib/views/reviews-footer-view.js @@ -0,0 +1,39 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + + +export default class ReviewsFooterView extends React.Component { + static propTypes = { + commentsResolved: PropTypes.number.isRequired, + totalComments: PropTypes.number.isRequired, + }; + + render() { + return ( +
+ + Reviews + + + + Resolved{' '} + + {this.props.commentsResolved} + + {' '}of{' '} + + {this.props.totalComments} + {' '}comments + + + {' '}comments{' '} + + + + +
+ ); + } +} From 0efc6a5c4ad565cac49d800a1bd681135d9ed36f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 14:49:52 -0500 Subject: [PATCH 009/738] Serialization and deserialization for ReviewsItems --- lib/github-package.js | 10 ++++++++++ lib/items/reviews-item.js | 2 +- package.json | 3 ++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index 092c8eb6fa..ae96b0fa0c 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -394,6 +394,16 @@ export default class GithubPackage { return item; } + createReviewsStub({uri}) { + const item = StubItem.create('github-reviews', { + title: 'Reviews', + }, uri); + if (this.controller) { + this.rerender(); + } + return item; + } + destroyGitTabItem() { if (this.gitTabStubItem) { this.gitTabStubItem.destroy(); diff --git a/lib/items/reviews-item.js b/lib/items/reviews-item.js index f8fbb0df01..95369545b7 100644 --- a/lib/items/reviews-item.js +++ b/lib/items/reviews-item.js @@ -77,7 +77,7 @@ export default class ReviewsItem extends React.Component { serialize() { return { deserializer: 'ReviewsStub', - uri: this.constructor.buildURI( + uri: ReviewsItem.buildURI( this.props.host, this.props.owner, this.props.repo, diff --git a/package.json b/package.json index 056387db25..f6293da2b1 100644 --- a/package.json +++ b/package.json @@ -204,7 +204,8 @@ "GithubDockItem": "createDockItemStub", "FilePatchControllerStub": "createFilePatchControllerStub", "CommitPreviewStub": "createCommitPreviewStub", - "CommitDetailStub": "createCommitDetailStub" + "CommitDetailStub": "createCommitDetailStub", + "ReviewsStub": "createReviewsStub" }, "greenkeeper": { "ignore": [ From 3a4aee94edf793392cc74025a837ab5100fea8f5 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 14:50:20 -0500 Subject: [PATCH 010/738] Plumbing to open the ReviewsItem from the ReviewsFooterView --- lib/controllers/issueish-detail-controller.js | 22 +++++++++++++++++++ lib/views/pr-detail-view.js | 7 +++++- lib/views/reviews-footer-view.js | 5 ++++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/controllers/issueish-detail-controller.js b/lib/controllers/issueish-detail-controller.js index c704a8ebed..5beb3642a5 100644 --- a/lib/controllers/issueish-detail-controller.js +++ b/lib/controllers/issueish-detail-controller.js @@ -10,6 +10,7 @@ import EnableableOperation from '../models/enableable-operation'; import PullRequestDetailView, {checkoutStates} from '../views/pr-detail-view'; import IssueDetailView from '../views/issue-detail-view'; import CommitDetailItem from '../items/commit-detail-item'; +import ReviewsItem from '../items/reviews-item'; import {incrementCounter, addEvent} from '../reporter-proxy'; export class BareIssueishDetailController extends React.Component { @@ -132,6 +133,7 @@ export class BareIssueishDetailController extends React.Component { repository={repository} pullRequest={repository.pullRequest} checkoutOp={this.checkoutOp} + openReviews={this.openReviews} switchToIssueish={this.props.switchToIssueish} endpoint={this.props.endpoint} @@ -290,6 +292,26 @@ export class BareIssueishDetailController extends React.Component { await this.props.workspace.open(uri, {pending: true}); addEvent('open-commit-in-pane', {package: 'github', from: this.constructor.name}); } + + openReviews = async () => { + if (!this.props.workdirPath) { + return; + } + + if (this.state.typename !== 'PullRequest') { + return; + } + + const uri = ReviewsItem.buildURI( + this.props.endpoint.getHost(), + this.props.repository.owner.login, + this.props.repository.name, + this.props.issueishNumber, + this.props.workdirPath, + ); + await this.props.workspace.open(uri); + addEvent('open-reviews-tab', {package: 'github', from: this.constructor.name}); + } } export default createFragmentContainer(BareIssueishDetailController, { diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 794842b55a..1f8cc76955 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -41,6 +41,7 @@ export class BarePullRequestDetailView extends React.Component { relay: PropTypes.shape({ refetch: PropTypes.func.isRequired, }), + openReviews: PropTypes.func.isRequired, switchToIssueish: PropTypes.func.isRequired, checkoutOp: EnableableOperationPropType.isRequired, repository: PropTypes.shape({ @@ -276,7 +277,11 @@ export class BarePullRequestDetailView extends React.Component { {this.renderPullRequestBody(pullRequest)} - + ); diff --git a/lib/views/reviews-footer-view.js b/lib/views/reviews-footer-view.js index 1e444f2943..8ad8a671ed 100644 --- a/lib/views/reviews-footer-view.js +++ b/lib/views/reviews-footer-view.js @@ -6,6 +6,9 @@ export default class ReviewsFooterView extends React.Component { static propTypes = { commentsResolved: PropTypes.number.isRequired, totalComments: PropTypes.number.isRequired, + + // Controller actions + openReviews: PropTypes.func.isRequired, }; render() { @@ -31,7 +34,7 @@ export default class ReviewsFooterView extends React.Component { + onClick={this.props.openReviews}>Open reviews ); From 78a6c64566036ce8f02b2bf3b306f640591a6988 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 14:53:27 -0500 Subject: [PATCH 011/738] Generate Relay files for the stub query in ReviewsContainer --- .../reviewsContainerQuery.graphql.js | 114 ++++++++++++++++++ lib/containers/reviews-container.js | 10 +- 2 files changed, 119 insertions(+), 5 deletions(-) create mode 100644 lib/containers/__generated__/reviewsContainerQuery.graphql.js diff --git a/lib/containers/__generated__/reviewsContainerQuery.graphql.js b/lib/containers/__generated__/reviewsContainerQuery.graphql.js new file mode 100644 index 0000000000..df5eb0e96a --- /dev/null +++ b/lib/containers/__generated__/reviewsContainerQuery.graphql.js @@ -0,0 +1,114 @@ +/** + * @flow + * @relayHash 68140a8082e21c7d56516fb6ca6c8db7 + */ + +/* eslint-disable */ + +'use strict'; + +/*:: +import type { ConcreteRequest } from 'relay-runtime'; +export type reviewsContainerQueryVariables = {| + repoOwner: string, + repoName: string, +|}; +export type reviewsContainerQueryResponse = {| + +repository: ?{| + +id: string + |} +|}; +export type reviewsContainerQuery = {| + variables: reviewsContainerQueryVariables, + response: reviewsContainerQueryResponse, +|}; +*/ + + +/* +query reviewsContainerQuery( + $repoOwner: String! + $repoName: String! +) { + repository(owner: $repoOwner, name: $repoName) { + id + } +} +*/ + +const node/*: ConcreteRequest*/ = (function(){ +var v0 = [ + { + "kind": "LocalArgument", + "name": "repoOwner", + "type": "String!", + "defaultValue": null + }, + { + "kind": "LocalArgument", + "name": "repoName", + "type": "String!", + "defaultValue": null + } +], +v1 = [ + { + "kind": "LinkedField", + "alias": null, + "name": "repository", + "storageKey": null, + "args": [ + { + "kind": "Variable", + "name": "name", + "variableName": "repoName", + "type": "String!" + }, + { + "kind": "Variable", + "name": "owner", + "variableName": "repoOwner", + "type": "String!" + } + ], + "concreteType": "Repository", + "plural": false, + "selections": [ + { + "kind": "ScalarField", + "alias": null, + "name": "id", + "args": null, + "storageKey": null + } + ] + } +]; +return { + "kind": "Request", + "fragment": { + "kind": "Fragment", + "name": "reviewsContainerQuery", + "type": "Query", + "metadata": null, + "argumentDefinitions": (v0/*: any*/), + "selections": (v1/*: any*/) + }, + "operation": { + "kind": "Operation", + "name": "reviewsContainerQuery", + "argumentDefinitions": (v0/*: any*/), + "selections": (v1/*: any*/) + }, + "params": { + "operationKind": "query", + "name": "reviewsContainerQuery", + "id": null, + "text": "query reviewsContainerQuery(\n $repoOwner: String!\n $repoName: String!\n) {\n repository(owner: $repoOwner, name: $repoName) {\n id\n }\n}\n", + "metadata": {} + } +}; +})(); +// prettier-ignore +(node/*: any*/).hash = '2a432f9e2a062215c96118ec0b5070cd'; +module.exports = node; diff --git a/lib/containers/reviews-container.js b/lib/containers/reviews-container.js index 7de75d094e..e0e1b4310c 100644 --- a/lib/containers/reviews-container.js +++ b/lib/containers/reviews-container.js @@ -74,11 +74,11 @@ export default class ReviewsContainer extends React.Component { ( $repoOwner: String! $repoName: String! - $prNumber: Int! - $reviewCount: Int! - $reviewCursor: String - $commentCount: Int! - $commentCursor: String + # $prNumber: Int! + # $reviewCount: Int! + # $reviewCursor: String + # $commentCount: Int! + # $commentCursor: String ) { repository(owner: $repoOwner, name: $repoName) { id From 41b88dcdcafad28c623c7f873de896c10836376e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 15:01:44 -0500 Subject: [PATCH 012/738] fetchToken() returns the token alone, not wrapped in an object --- lib/containers/reviews-container.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/containers/reviews-container.js b/lib/containers/reviews-container.js index e0e1b4310c..81cd707a27 100644 --- a/lib/containers/reviews-container.js +++ b/lib/containers/reviews-container.js @@ -37,16 +37,16 @@ export default class ReviewsContainer extends React.Component { ); } - renderWithToken(tokenData) { - if (!tokenData) { + renderWithToken(token) { + if (!token) { return ; } - if (tokenData.token === UNAUTHENTICATED) { + if (token === UNAUTHENTICATED) { return ; } - if (tokenData.token === INSUFFICIENT) { + if (token === INSUFFICIENT) { return (

@@ -58,7 +58,7 @@ export default class ReviewsContainer extends React.Component { return ( - {repoData => this.renderWithRepositoryData(repoData, tokenData.token)} + {repoData => this.renderWithRepositoryData(repoData, token)} ); } @@ -143,5 +143,5 @@ export default class ReviewsContainer extends React.Component { handleLogin = token => this.props.loginModel.setToken(this.props.endpoint.getLoginAccount(), token); - handleLogout = this.props.loginModel.removeToken(this.props.endpoint.getLoginAccount()); + handleLogout = () => this.props.loginModel.removeToken(this.props.endpoint.getLoginAccount()); } From 062e20e84235946dd201d79bb89e10aa8ca9e694 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 15:38:36 -0500 Subject: [PATCH 013/738] Stub tests for ReviewsItem --- test/items/reviews-item.test.js | 110 ++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 test/items/reviews-item.test.js diff --git a/test/items/reviews-item.test.js b/test/items/reviews-item.test.js new file mode 100644 index 0000000000..f5fb363f31 --- /dev/null +++ b/test/items/reviews-item.test.js @@ -0,0 +1,110 @@ +import React from 'react'; +import {mount} from 'enzyme'; + +import ReviewsItem from '../../lib/items/reviews-item'; +import {cloneRepository} from '../helpers'; +import PaneItem from '../../lib/atom/pane-item'; +import {InMemoryStrategy} from '../../lib/shared/keytar-strategy'; +import GithubLoginModel from '../../lib/models/github-login-model'; +import WorkdirContextPool from '../../lib/models/workdir-context-pool'; + +describe('ReviewsItem', function() { + let atomEnv, repository, pool; + + beforeEach(async function() { + atomEnv = global.buildAtomEnvironment(); + const workdir = await cloneRepository(); + + pool = new WorkdirContextPool({ + workspace: atomEnv.workspace, + }); + + repository = pool.add(workdir).getRepository(); + }); + + afterEach(function() { + atomEnv.destroy(); + pool.clear(); + }); + + function buildPaneApp(override = {}) { + const props = { + workdirContextPool: pool, + loginModel: new GithubLoginModel(InMemoryStrategy), + ...override, + }; + + return ( + + {({itemHolder, params}) => ( + + )} + + ); + } + + async function open(wrapper, options = {}) { + const opts = { + host: 'github.com', + owner: 'atom', + repo: 'github', + number: 1848, + workdir: repository.getWorkingDirectoryPath(), + ...options, + }; + + const uri = ReviewsItem.buildURI(opts.host, opts.owner, opts.repo, opts.number, opts.workdir); + const item = await atomEnv.workspace.open(uri); + wrapper.update(); + return item; + } + + it('constructs and opens the correct URI', async function() { + const wrapper = mount(buildPaneApp()); + assert.isFalse(wrapper.exists('ReviewsItem')); + await open(wrapper); + assert.isTrue(wrapper.exists('ReviewsItem')); + }); + + it('locates the repository from the context pool', async function() { + const wrapper = mount(buildPaneApp()); + await open(wrapper); + + assert.strictEqual(wrapper.find('ReviewsContainer').prop('repository'), repository); + }); + + it('returns a title containing the pull request number', async function() { + const wrapper = mount(buildPaneApp()); + const item = await open(wrapper, {number: 1234}); + + assert.strictEqual(item.getTitle(), 'Reviews #1234'); + }); + + it('may be destroyed once', async function() { + const wrapper = mount(buildPaneApp()); + + const item = await open(wrapper); + const callback = sinon.spy(); + const sub = item.onDidDestroy(callback); + + assert.strictEqual(callback.callCount, 0); + item.destroy(); + assert.strictEqual(callback.callCount, 1); + + sub.dispose(); + }); + + it('serializes itself as a ReviewsItemStub', async function() { + const wrapper = mount(buildPaneApp()); + const item = await open(wrapper, {host: 'github.horse', owner: 'atom', repo: 'atom', number: 12, workdir: '/here'}); + assert.deepEqual(item.serialize(), { + deserializer: 'ReviewsStub', + uri: 'atom-github://reviews/github.horse/atom/atom/12?workdir=%2Fhere', + }); + }); +}); From a281f7061e379622707f7436becb955bc7f722bc Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 15:45:24 -0500 Subject: [PATCH 014/738] Stub test for ReviewsContainer --- test/containers/reviews-container.test.js | 46 +++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 test/containers/reviews-container.test.js diff --git a/test/containers/reviews-container.test.js b/test/containers/reviews-container.test.js new file mode 100644 index 0000000000..77075e6c85 --- /dev/null +++ b/test/containers/reviews-container.test.js @@ -0,0 +1,46 @@ +import React from 'react'; +import {mount} from 'enzyme'; + +import ReviewsContainer from '../../lib/containers/reviews-container'; +import Repository from '../../lib/models/repository'; +import {InMemoryStrategy} from '../../lib/shared/keytar-strategy'; +import GithubLoginModel from '../../lib/models/github-login-model'; +import {getEndpoint} from '../../lib/models/endpoint'; +import {cloneRepository} from '../helpers'; + +describe('ReviewsContainer', function() { + let atomEnv, repository, loginModel; + + beforeEach(async function() { + atomEnv = global.buildAtomEnvironment(); + const workdir = await cloneRepository(); + repository = new Repository(workdir); + await repository.getLoadPromise(); + loginModel = new GithubLoginModel(InMemoryStrategy); + }); + + afterEach(function() { + atomEnv.destroy(); + }); + + function buildApp(override = {}) { + const props = { + endpoint: getEndpoint('github.com'), + owner: 'atom', + repo: 'github', + number: 1234, + repository, + loginModel, + ...override, + }; + + return ; + } + + it('renders a loading spinner while the token is loading', function() { + sinon.stub(loginModel, 'getToken').returns(new Promise(() => {})); + + const wrapper = mount(buildApp()); + assert.isTrue(wrapper.exists('LoadingView')); + }); +}); From 99162ced2143aa68fb98a2dde405db81ad97d3cd Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 15:47:16 -0500 Subject: [PATCH 015/738] Stub test for ReviewsController --- test/controllers/reviews-controller.test.js | 31 +++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 test/controllers/reviews-controller.test.js diff --git a/test/controllers/reviews-controller.test.js b/test/controllers/reviews-controller.test.js new file mode 100644 index 0000000000..273ef953c2 --- /dev/null +++ b/test/controllers/reviews-controller.test.js @@ -0,0 +1,31 @@ +import React from 'react'; +import {shallow} from 'enzyme'; + +import ReviewsController from '../../lib/controllers/reviews-controller'; + +describe('ReviewsController', function() { + let atomEnv; + + beforeEach(function() { + atomEnv = global.buildAtomEnvironment(); + }); + + afterEach(function() { + atomEnv.destroy(); + }); + + function buildApp(override = {}) { + const props = { + ...override, + }; + + return ; + } + + it('renders a ReviewsView', function() { + const extra = Symbol('extra'); + const wrapper = shallow(buildApp({extra})); + assert.isTrue(wrapper.exists('ReviewsView')); + assert.strictEqual(wrapper.find('ReviewsView').prop('extra'), extra); + }); +}); From cc5a2479e1e639a3b0ac5117704488d014dd57ce Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 15:48:47 -0500 Subject: [PATCH 016/738] Stub test for ReviewsView --- test/views/reviews-view.test.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 test/views/reviews-view.test.js diff --git a/test/views/reviews-view.test.js b/test/views/reviews-view.test.js new file mode 100644 index 0000000000..736627fc22 --- /dev/null +++ b/test/views/reviews-view.test.js @@ -0,0 +1,29 @@ +import React from 'react'; +import {shallow} from 'enzyme'; + +import ReviewsView from '../../lib/views/reviews-view'; + +describe('ReviewsView', function() { + let atomEnv; + + beforeEach(function() { + atomEnv = global.buildAtomEnvironment(); + }); + + afterEach(function() { + atomEnv.destroy(); + }); + + function buildApp(override = {}) { + const props = { + ...override, + }; + + return ; + } + + it('renders something', function() { + const wrapper = shallow(buildApp()); + assert.isTrue(wrapper.exists('div')); + }); +}); From f0ccde066ed183f772b4fef209bab22c922aaa62 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 1 Mar 2019 15:54:01 -0500 Subject: [PATCH 017/738] Stub test for ReviewsFooterView --- test/views/reviews-footer-view.test.js | 44 ++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 test/views/reviews-footer-view.test.js diff --git a/test/views/reviews-footer-view.test.js b/test/views/reviews-footer-view.test.js new file mode 100644 index 0000000000..d4eed8b8d9 --- /dev/null +++ b/test/views/reviews-footer-view.test.js @@ -0,0 +1,44 @@ +import React from 'react'; +import {shallow} from 'enzyme'; + +import ReviewsFooterView from '../../lib/views/reviews-footer-view'; + +describe('ReviewsFooterView', function() { + let atomEnv; + + beforeEach(function() { + atomEnv = global.buildAtomEnvironment(); + }); + + afterEach(function() { + atomEnv.destroy(); + }); + + function buildApp(override = {}) { + const props = { + commentsResolved: 3, + totalComments: 4, + ...override, + }; + + return ; + } + + it('renders the resolved and total comment counts', function() { + const wrapper = shallow(buildApp({commentsResolved: 4, totalComments: 7})); + + assert.strictEqual(wrapper.find('.github-PrDetailViewReviews-countNr').text(), '4'); + assert.strictEqual(wrapper.find('.github-Reviews-countNr').text(), '7'); + assert.strictEqual(wrapper.find('.github-PrDetailViewReviews-progessBar').prop('value'), 4); + assert.strictEqual(wrapper.find('.github-PrDetailViewReviews-progessBar').prop('max'), 7); + }); + + it('triggers openReviews on button click', function() { + const openReviews = sinon.spy(); + const wrapper = shallow(buildApp({openReviews})); + + wrapper.find('.github-PrDetailViewReviews-openReviewsButton').simulate('click'); + + assert.isTrue(openReviews.called); + }); +}); From c50a80a5c9acac73c9d3775e57fb79e42578078c Mon Sep 17 00:00:00 2001 From: annthurium Date: Fri, 1 Mar 2019 13:47:58 -0800 Subject: [PATCH 018/738] style the :foot:er is my emoji too much? SORRY NOT SORRY --- lib/views/reviews-footer-view.js | 23 ++++++++----------- styles/reviews-footer-view.less | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 13 deletions(-) create mode 100644 styles/reviews-footer-view.less diff --git a/lib/views/reviews-footer-view.js b/lib/views/reviews-footer-view.js index 8ad8a671ed..d693d8dcea 100644 --- a/lib/views/reviews-footer-view.js +++ b/lib/views/reviews-footer-view.js @@ -6,36 +6,33 @@ export default class ReviewsFooterView extends React.Component { static propTypes = { commentsResolved: PropTypes.number.isRequired, totalComments: PropTypes.number.isRequired, - - // Controller actions - openReviews: PropTypes.func.isRequired, }; render() { return ( -

- +
+ Reviews - - + + Resolved{' '} - + {this.props.commentsResolved} {' '}of{' '} - + {this.props.totalComments} {' '}comments - {' '}comments{' '} - - + +
); } diff --git a/styles/reviews-footer-view.less b/styles/reviews-footer-view.less new file mode 100644 index 0000000000..6b24b82ad0 --- /dev/null +++ b/styles/reviews-footer-view.less @@ -0,0 +1,39 @@ +@import 'variables'; + +.github-ReviewsFooterView { + + margin-right: auto; + + // Footer ------------------------ + + &-footer { + display: flex; + align-items: center; + padding: @component-padding; + border-top: 1px solid @base-border-color; + background-color: @app-background-color; + } + + &-footerTitle { + font-size: 1.4em; + margin: 0 @component-padding*2 0 0; + } + + &-openReviewsButton, + &-reviewChangesButton { + margin-left: @component-padding; + } + + &-count { + margin-right: @component-padding; + color: @text-color-subtle; + } + + &-countNumber { + color: @text-color-highlight; + } + + &-progessBar { + margin-right: @component-padding; + } +} From e85700cbc170802f94d0a99503c0c1d473a5c164 Mon Sep 17 00:00:00 2001 From: annthurium Date: Fri, 1 Mar 2019 13:50:34 -0800 Subject: [PATCH 019/738] fix @smaswilson's changes that I accidentally nerfed Co-Authored-By: Ash Wilson --- lib/views/reviews-footer-view.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/views/reviews-footer-view.js b/lib/views/reviews-footer-view.js index d693d8dcea..f8abded9c1 100644 --- a/lib/views/reviews-footer-view.js +++ b/lib/views/reviews-footer-view.js @@ -6,6 +6,9 @@ export default class ReviewsFooterView extends React.Component { static propTypes = { commentsResolved: PropTypes.number.isRequired, totalComments: PropTypes.number.isRequired, + + // Controller actions + openReviews: PropTypes.func.isRequired, }; render() { @@ -31,7 +34,7 @@ export default class ReviewsFooterView extends React.Component { + onClick={this.props.openReviews}>Open reviews
); From 2e4bb46819f229ab1251208541c701c6f1dd3fc7 Mon Sep 17 00:00:00 2001 From: annthurium Date: Fri, 1 Mar 2019 14:11:11 -0800 Subject: [PATCH 020/738] fix ReviewsFooterView tests --- lib/views/reviews-footer-view.js | 6 +++--- styles/reviews-footer-view.less | 4 ++-- test/views/reviews-footer-view.test.js | 11 ++++++----- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/views/reviews-footer-view.js b/lib/views/reviews-footer-view.js index f8abded9c1..82f41a8155 100644 --- a/lib/views/reviews-footer-view.js +++ b/lib/views/reviews-footer-view.js @@ -18,13 +18,13 @@ export default class ReviewsFooterView extends React.Component { Reviews - + Resolved{' '} - + {this.props.commentsResolved} {' '}of{' '} - + {this.props.totalComments} {' '}comments diff --git a/styles/reviews-footer-view.less b/styles/reviews-footer-view.less index 6b24b82ad0..4a3179b19b 100644 --- a/styles/reviews-footer-view.less +++ b/styles/reviews-footer-view.less @@ -24,12 +24,12 @@ margin-left: @component-padding; } - &-count { + &-commentCount { margin-right: @component-padding; color: @text-color-subtle; } - &-countNumber { + &-commentsResolved { color: @text-color-highlight; } diff --git a/test/views/reviews-footer-view.test.js b/test/views/reviews-footer-view.test.js index d4eed8b8d9..03339076f3 100644 --- a/test/views/reviews-footer-view.test.js +++ b/test/views/reviews-footer-view.test.js @@ -18,6 +18,7 @@ describe('ReviewsFooterView', function() { const props = { commentsResolved: 3, totalComments: 4, + openReviews: () => {}, ...override, }; @@ -27,17 +28,17 @@ describe('ReviewsFooterView', function() { it('renders the resolved and total comment counts', function() { const wrapper = shallow(buildApp({commentsResolved: 4, totalComments: 7})); - assert.strictEqual(wrapper.find('.github-PrDetailViewReviews-countNr').text(), '4'); - assert.strictEqual(wrapper.find('.github-Reviews-countNr').text(), '7'); - assert.strictEqual(wrapper.find('.github-PrDetailViewReviews-progessBar').prop('value'), 4); - assert.strictEqual(wrapper.find('.github-PrDetailViewReviews-progessBar').prop('max'), 7); + assert.strictEqual(wrapper.find('.github-ReviewsFooterView-commentsResolved').text(), '4'); + assert.strictEqual(wrapper.find('.github-ReviewsFooterView-totalComments').text(), '7'); + assert.strictEqual(wrapper.find('.github-ReviewsFooterView-progessBar').prop('value'), 4); + assert.strictEqual(wrapper.find('.github-ReviewsFooterView-progessBar').prop('max'), 7); }); it('triggers openReviews on button click', function() { const openReviews = sinon.spy(); const wrapper = shallow(buildApp({openReviews})); - wrapper.find('.github-PrDetailViewReviews-openReviewsButton').simulate('click'); + wrapper.find('.github-ReviewsFooterView-openReviewsButton').simulate('click'); assert.isTrue(openReviews.called); }); From 4b6787059fd4854e1ec3b8f50c1f626b95823cd9 Mon Sep 17 00:00:00 2001 From: annthurium Date: Fri, 1 Mar 2019 14:24:40 -0800 Subject: [PATCH 021/738] add test for footer in `PullRequestDetailView` --- test/fixtures/props/issueish-pane-props.js | 1 + test/views/pr-detail-view.test.js | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/test/fixtures/props/issueish-pane-props.js b/test/fixtures/props/issueish-pane-props.js index cabc7e4140..4fd32e976d 100644 --- a/test/fixtures/props/issueish-pane-props.js +++ b/test/fixtures/props/issueish-pane-props.js @@ -171,6 +171,7 @@ export function pullRequestDetailViewProps(opts, overrides = {}) { switchToIssueish: () => {}, destroy: () => {}, openCommit: () => {}, + openReviews: () => {}, // atom env props workspace: {}, diff --git a/test/views/pr-detail-view.test.js b/test/views/pr-detail-view.test.js index 62364fe02d..99f7ec1e8f 100644 --- a/test/views/pr-detail-view.test.js +++ b/test/views/pr-detail-view.test.js @@ -64,6 +64,14 @@ describe('PullRequestDetailView', function() { assert.strictEqual(wrapper.find('.github-IssueishDetailView-headRefName').text(), headRefName); }); + it('renders footer and passes openReviews prop through', function() { + const wrapper = shallow(buildApp()); + const footer = wrapper.find('ReviewsFooterView'); + assert.lengthOf(footer, 1); + + assert.strictEqual(footer.prop('openReviews'), wrapper.instance().props.openReviews); + }); + it('renders tabs', function() { const pullRequestCommitCount = 11; const pullRequestChangedFileCount = 22; From 1935f455904ed552a0d5365e3c7eca1120aa2168 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 10:23:50 -0500 Subject: [PATCH 022/738] :art: delete a newline --- lib/containers/pr-review-comments-container.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/containers/pr-review-comments-container.js b/lib/containers/pr-review-comments-container.js index 16051026bd..03749fee74 100644 --- a/lib/containers/pr-review-comments-container.js +++ b/lib/containers/pr-review-comments-container.js @@ -5,7 +5,6 @@ import React from 'react'; import {PAGE_SIZE, PAGINATION_WAIT_TIME_MS} from '../helpers'; export class BarePullRequestReviewCommentsContainer extends React.Component { - static propTypes = { collectComments: PropTypes.func.isRequired, review: PropTypes.shape({ From 9bec06acb195214e25115d5e13c1661a567293f5 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 10:26:53 -0500 Subject: [PATCH 023/738] PullRequestReviewsController calls a render prop with each comment thread This sets us up to render a PullRequestCommentThreadView from MultiFilePatchView and a different view component from the ReviewsItem. --- lib/controllers/pr-reviews-controller.js | 16 ++-- .../controllers/pr-reviews-controller.test.js | 93 ++++++++++++++++--- 2 files changed, 88 insertions(+), 21 deletions(-) diff --git a/lib/controllers/pr-reviews-controller.js b/lib/controllers/pr-reviews-controller.js index fdaf09c434..94adeed5a5 100644 --- a/lib/controllers/pr-reviews-controller.js +++ b/lib/controllers/pr-reviews-controller.js @@ -3,7 +3,6 @@ import PropTypes from 'prop-types'; import {RelayConnectionPropType} from '../prop-types'; import PullRequestReviewCommentsContainer from '../containers/pr-review-comments-container'; -import PullRequestReviewCommentsView from '../views/pr-review-comments-view'; import {PAGE_SIZE, PAGINATION_WAIT_TIME_MS} from '../helpers'; export default class PullRequestReviewsController extends React.Component { @@ -18,8 +17,7 @@ export default class PullRequestReviewsController extends React.Component { PropTypes.object, ), }), - getBufferRowForDiffPosition: PropTypes.func.isRequired, - isPatchVisible: PropTypes.func.isRequired, + children: PropTypes.func.isRequired, } constructor(props) { @@ -77,12 +75,18 @@ export default class PullRequestReviewsController extends React.Component { * Upon fetching new comments, the `collectComments` method is called with all comments fetched for that review. * Ultimately we want to group comments based on the root comment they are replies to. * `renderCommentFetchingContainers` only fetches data and doesn't render any user visible DOM elements. - * `PullRequestReviewCommentsView` renders the aggregated comment thread data. - * */ + * The child render prop is called with each comment thread. + */ return ( {this.renderCommentFetchingContainers()} - + {commentThreads.map(commentThread => { + return ( + + {this.props.children(commentThread)} + + ); + })} ); } diff --git a/test/controllers/pr-reviews-controller.test.js b/test/controllers/pr-reviews-controller.test.js index 60e2dbf2be..e90c1d6ca9 100644 --- a/test/controllers/pr-reviews-controller.test.js +++ b/test/controllers/pr-reviews-controller.test.js @@ -1,17 +1,23 @@ import React from 'react'; import {shallow} from 'enzyme'; import {reviewBuilder} from '../builder/pr'; +import {inspect} from 'util'; import PullRequestReviewsController from '../../lib/controllers/pr-reviews-controller'; +import PullRequestReviewCommentsContainer from '../../lib/containers/pr-review-comments-container'; import {PAGE_SIZE, PAGINATION_WAIT_TIME_MS} from '../../lib/helpers'; +function SomeView(props) { + return 
{inspect(props, {depth: 3})}
; +} + describe('PullRequestReviewsController', function() { - function buildApp(opts, overrideProps = {}) { + function buildApp(opts, overrideProps = {}, childFn = SomeView) { const o = { - relayHasMore: () => { return false; }, + relayHasMore: () => false, relayLoadMore: () => {}, - relayIsLoading: () => { return false; }, + relayIsLoading: () => false, reviewSpecs: [], reviewStartCursor: 0, ...opts, @@ -40,14 +46,15 @@ describe('PullRequestReviewsController', function() { loadMore: o.relayLoadMore, isLoading: o.relayIsLoading, }, - - switchToIssueish: () => {}, - getBufferRowForDiffPosition: () => {}, - isPatchVisible: () => true, pullRequest: {reviews}, ...overrideProps, }; - return ; + + return ( + + {childFn} + + ); } it('returns null if props.pullRequest is falsy', function() { const wrapper = shallow(buildApp({}, {pullRequest: null})); @@ -72,17 +79,73 @@ describe('PullRequestReviewsController', function() { assert.strictEqual(containers.at(1).prop('review').id, review2.id); }); - it('renders a PullRequestReviewCommentsView and passes props through', function() { - const review1 = reviewBuilder().build(); - const review2 = reviewBuilder().build(); + it('calls the child render prop for each comment thread', function() { + const review1 = reviewBuilder() + .id(10) + .submittedAt('2019-01-01T10:00:00Z') + .addComment(c => c.id(1)) + .build(); + const review2 = reviewBuilder() + .id(20) + .submittedAt('2019-01-01T11:00:00Z') + .addComment(c => c.id(2)) + .addComment(c => c.id(3).replyTo(1)) + .addComment(c => c.id(4)) + .build(); + + function ChildComponent() { + return
; + } const reviewSpecs = [review1, review2]; const passThroughProp = 'I only exist for the children'; - const wrapper = shallow(buildApp({reviewSpecs}, {passThroughProp})); - const view = wrapper.find('PullRequestCommentsView'); - assert.strictEqual(view.length, 1); - assert.strictEqual(wrapper.instance().props.passThroughProp, view.prop('passThroughProp')); + const wrapper = shallow(buildApp({reviewSpecs}, {}, thread => { + return ; + })); + + assert.isFalse(wrapper.exists('ChildComponent')); + + wrapper.find(PullRequestReviewCommentsContainer).at(0).prop('collectComments')({ + reviewId: review1.id, + submittedAt: review1.submittedAt, + comments: review1.comments, + fetchingMoreComments: false, + }); + + assert.lengthOf(wrapper.find('ChildComponent'), 1); + assert.strictEqual(wrapper.find('ChildComponent').at(0).prop('passThroughProp'), passThroughProp); + assert.deepEqual(wrapper.find('ChildComponent').at(0).prop('thread'), { + rootCommentId: '1', + comments: [review1.comments.edges[0].node], + }); + + wrapper.find(PullRequestReviewCommentsContainer).at(1).prop('collectComments')({ + reviewId: review2.id, + submittedAt: review2.submittedAt, + comments: review2.comments, + fetchingMoreComments: false, + }); + + assert.lengthOf(wrapper.find('ChildComponent'), 3); + + assert.strictEqual(wrapper.find('ChildComponent').at(0).prop('passThroughProp'), passThroughProp); + assert.deepEqual(wrapper.find('ChildComponent').at(0).prop('thread'), { + rootCommentId: '4', + comments: [review2.comments.edges[2].node], + }); + + assert.strictEqual(wrapper.find('ChildComponent').at(1).prop('passThroughProp'), passThroughProp); + assert.deepEqual(wrapper.find('ChildComponent').at(1).prop('thread'), { + rootCommentId: '2', + comments: [review2.comments.edges[0].node], + }); + + assert.strictEqual(wrapper.find('ChildComponent').at(2).prop('passThroughProp'), passThroughProp); + assert.deepEqual(wrapper.find('ChildComponent').at(2).prop('thread'), { + rootCommentId: '1', + comments: [review1.comments.edges[0].node, review2.comments.edges[1].node], + }); }); describe('collectComments', function() { From 30bc7ced9d0ced734b14d21d417afde4b61d0dd1 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 10:28:34 -0500 Subject: [PATCH 024/738] PullRequestReviewCommentsView > PullRequestReviewCommentThreadView Rework PullRequestReviewCommentsView to render a single comment thread at a time. Rename it to PullRequestReviewCommentThreadView which is a mouthful, but a more accurate description of what it does. --- ...ew.js => pr-review-comment-thread-view.js} | 63 +++++++++---------- ... => pr-review-comment-thread-view.test.js} | 51 +++++++-------- 2 files changed, 54 insertions(+), 60 deletions(-) rename lib/views/{pr-review-comments-view.js => pr-review-comment-thread-view.js} (63%) rename test/views/{pr-comments-view.test.js => pr-review-comment-thread-view.test.js} (75%) diff --git a/lib/views/pr-review-comments-view.js b/lib/views/pr-review-comment-thread-view.js similarity index 63% rename from lib/views/pr-review-comments-view.js rename to lib/views/pr-review-comment-thread-view.js index 636b88d4b1..232136dca4 100644 --- a/lib/views/pr-review-comments-view.js +++ b/lib/views/pr-review-comment-thread-view.js @@ -10,55 +10,51 @@ import Octicon from '../atom/octicon'; import GithubDotcomMarkdown from './github-dotcom-markdown'; import Timeago from './timeago'; -export default class PullRequestCommentsView extends React.Component { +export default class PullRequestReviewCommentThreadView extends React.Component { static propTypes = { - commentThreads: PropTypes.arrayOf(PropTypes.shape({ + commentThread: PropTypes.shape({ rootCommentId: PropTypes.string.isRequired, comments: PropTypes.arrayOf(PropTypes.object).isRequired, - })), + }), getBufferRowForDiffPosition: PropTypes.func.isRequired, switchToIssueish: PropTypes.func.isRequired, isPatchVisible: PropTypes.func.isRequired, } render() { - return [...this.props.commentThreads].map(({rootCommentId, comments}) => { - const rootComment = comments[0]; - if (!rootComment.position) { - return null; - } + const {rootCommentId, comments} = this.props.commentThread; + const rootComment = comments[0]; + if (!rootComment.position) { + return null; + } - // if file patch is collapsed or too large, do not render the comments - if (!this.props.isPatchVisible(rootComment.path)) { - return null; - } + // if file patch is collapsed or too large, do not render the comments + if (!this.props.isPatchVisible(rootComment.path)) { + return null; + } - const nativePath = toNativePathSep(rootComment.path); - const row = this.props.getBufferRowForDiffPosition(nativePath, rootComment.position); - const point = new Point(row, 0); - const range = new Range(point, point); - return ( - - - {comments.map(comment => { - return ( - - ); - })} - - - ); - }); + const nativePath = toNativePathSep(rootComment.path); + const row = this.props.getBufferRowForDiffPosition(nativePath, rootComment.position); + const point = new Point(row, 0); + const range = new Range(point, point); + return ( + + + {comments.map(comment => ( + + ))} + + + ); } } export class PullRequestCommentView extends React.Component { static propTypes = { - switchToIssueish: PropTypes.func.isRequired, comment: PropTypes.shape({ author: PropTypes.shape({ avatarUrl: PropTypes.string, @@ -69,6 +65,7 @@ export class PullRequestCommentView extends React.Component { createdAt: PropTypes.string.isRequired, isMinimized: PropTypes.bool.isRequired, }).isRequired, + switchToIssueish: PropTypes.func.isRequired, } render() { diff --git a/test/views/pr-comments-view.test.js b/test/views/pr-review-comment-thread-view.test.js similarity index 75% rename from test/views/pr-comments-view.test.js rename to test/views/pr-review-comment-thread-view.test.js index ff563441fc..331c294143 100644 --- a/test/views/pr-comments-view.test.js +++ b/test/views/pr-review-comment-thread-view.test.js @@ -3,26 +3,21 @@ import {shallow} from 'enzyme'; import {multiFilePatchBuilder} from '../builder/patch'; import {pullRequestBuilder} from '../builder/pr'; -import PullRequestCommentsView, {PullRequestCommentView} from '../../lib/views/pr-review-comments-view'; - -describe('PullRequestCommentsView', function() { - function buildApp(multiFilePatch, pullRequest, overrideProps) { - const relay = { - hasMore: () => {}, - loadMore: () => {}, - isLoading: () => {}, - }; +import PullRequestReviewCommentThreadView, {PullRequestCommentView} from '../../lib/views/pr-review-comment-thread-view'; + +describe('PullRequestReviewCommentThreadView', function() { + function buildApp(multiFilePatch, commentThread, overrideProps = {}) { return shallow( - true} + getBufferRowForDiffPosition={multiFilePatch.getBufferRowForDiffPosition} + commentThread={commentThread} switchToIssueish={() => {}} {...overrideProps} />, ); } + it('adjusts the position for comments after hunk headers', function() { const {multiFilePatch} = multiFilePatchBuilder() .addFilePatch(fp => { @@ -48,11 +43,14 @@ describe('PullRequestCommentsView', function() { }) .build(); - const wrapper = buildApp(multiFilePatch, pr, {}); + const wrapper0 = buildApp(multiFilePatch, pr.commentThreads[0]); + assert.deepEqual(wrapper0.find('Marker').prop('bufferRange').serialize(), [[1, 0], [1, 0]]); - assert.deepEqual(wrapper.find('Marker').at(0).prop('bufferRange').serialize(), [[1, 0], [1, 0]]); - assert.deepEqual(wrapper.find('Marker').at(1).prop('bufferRange').serialize(), [[12, 0], [12, 0]]); - assert.deepEqual(wrapper.find('Marker').at(2).prop('bufferRange').serialize(), [[20, 0], [20, 0]]); + const wrapper1 = buildApp(multiFilePatch, pr.commentThreads[1]); + assert.deepEqual(wrapper1.find('Marker').prop('bufferRange').serialize(), [[12, 0], [12, 0]]); + + const wrapper2 = buildApp(multiFilePatch, pr.commentThreads[2]); + assert.deepEqual(wrapper2.find('Marker').prop('bufferRange').serialize(), [[20, 0], [20, 0]]); }); it('does not render comment if patch is too large or collapsed', function() { @@ -64,9 +62,8 @@ describe('PullRequestCommentsView', function() { }) .build(); - const wrapper = buildApp(multiFilePatch, pr, {isPatchVisible: () => { return false; }}); - const comments = wrapper.find('PullRequestCommentView'); - assert.lengthOf(comments, 0); + const wrapper = buildApp(multiFilePatch, pr.commentThreads[0], {isPatchVisible: () => false}); + assert.isFalse(wrapper.exists('PullRequestCommentView')); }); it('does not render comment if position is null', function() { @@ -88,11 +85,11 @@ describe('PullRequestCommentsView', function() { }) .build(); - const wrapper = buildApp(multiFilePatch, pr, {}); + const wrapper = buildApp(multiFilePatch, pr.commentThreads[0]); const comments = wrapper.find('PullRequestCommentView'); assert.lengthOf(comments, 1); - assert.strictEqual(comments.at(0).prop('comment').body, 'one'); + assert.strictEqual(comments.prop('comment').body, 'one'); }); }); @@ -132,7 +129,7 @@ describe('PullRequestCommentView', function() { assert.strictEqual(avatar.getElement('src').props.src, avatarUrl); assert.strictEqual(avatar.getElement('alt').props.alt, login); - assert.isTrue(wrapper.text().includes(`${login} commented`)); + assert.include(wrapper.text(), `${login} commented`); const a = wrapper.find('.github-PrComment-timeAgo'); assert.strictEqual(a.getElement('href').props.href, commentUrl); @@ -147,12 +144,12 @@ describe('PullRequestCommentView', function() { it('contains the text `someone commented` for null authors', function() { const wrapper = shallow(buildApp({author: null})); - assert.isTrue(wrapper.text().includes('someone commented')); + assert.include(wrapper.text(), 'someone commented'); }); it('hides minimized comment', function() { const wrapper = shallow(buildApp({isMinimized: true})); - assert.isTrue(wrapper.find('.github-PrComment-hidden').exists()); - assert.isFalse(wrapper.find('.github-PrComment-header').exists()); + assert.isTrue(wrapper.exists('.github-PrComment-hidden')); + assert.isFalse(wrapper.exists('.github-PrComment-header')); }); }); From 9090283a478d6d61c613a0585c238d10d872b86d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 10:31:16 -0500 Subject: [PATCH 025/738] Pass a render prop to PullRequestReviewsContainer Preserve existing review comments behavior in MultiFilePatchView. --- lib/views/multi-file-patch-view.js | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/views/multi-file-patch-view.js b/lib/views/multi-file-patch-view.js index 76fa7e199f..edc80adb59 100644 --- a/lib/views/multi-file-patch-view.js +++ b/lib/views/multi-file-patch-view.js @@ -16,7 +16,8 @@ import Commands, {Command} from '../atom/commands'; import FilePatchHeaderView from './file-patch-header-view'; import FilePatchMetaView from './file-patch-meta-view'; import HunkHeaderView from './hunk-header-view'; -import PullRequestsReviewsContainer from '../containers/pr-reviews-container'; +import PullRequestReviewsContainer from '../containers/pr-reviews-container'; +import PullRequestReviewCommentThreadView from './pr-review-comment-thread-view'; import RefHolder from '../models/ref-holder'; import ChangedFileItem from '../items/changed-file-item'; import CommitDetailItem from '../items/commit-detail-item'; @@ -385,13 +386,16 @@ export default class MultiFilePatchView extends React.Component { // "forceRerender" ensures that the PullRequestCommentsView re-renders each time that the MultiFilePatchView does. // It doesn't re-query for reviews, but it does re-check patch visibility. return ( - + + {commentThread => ( + + )} + ); } else { return null; From eb70cc00cee5274b61b1e3954c88b18c690c99d8 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Mon, 4 Mar 2019 15:52:05 +0100 Subject: [PATCH 026/738] port over view styles from prototype --- lib/views/reviews-view.js | 394 +++++++++++++++++++++++++++++++++++++- styles/review.less | 388 +++++++++++++++++++++++++++++++++++++ 2 files changed, 779 insertions(+), 3 deletions(-) create mode 100644 styles/review.less diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index ce8ec6b45d..309581d925 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -4,9 +4,397 @@ import {inspect} from 'util'; export default class ReviewsView extends React.Component { render() { return ( -
-

HELL YEAH PR REVIEW COMMENTS

- 
{inspect(this.props, {depth: 4})}
+
+ + {/* Review Summaries ------------------------------------------ */} + +
+ +

Reviews

+ +
+ +
+
+ + + vanessayuenn + approved these changes + 18 minutes ago +
+
+ looks good! 🚢 +
+
+ +
+
+ + + simurai + requested changes + 2 hours ago +
+
+ This is a great addition. Before merging we might should rethink the position of the icon. +
+
+ +
+
+ + + annthurium + commented + 4 hours ago +
+
+ looking good so far -- just found a few nits to address. +
+
+ +
+
+ + + + {/* Review Comments ------------------------------------------ */} + +
+ +

Review comments

+ + Resolved 1 of 7 + + + +
+ +
+ + + lib/controllers/ + commit-detail-controller.js + 19 + + 4h + + + 
+                
{ '      messageCollapsible: this.props.commit.isBodyLong(),' }

+                
{ '      messageOpen: !this.props.commit.isBodyLong(),' }

+                
{ '    };' }

+                
{ '    }' }

+
+
+
+
+ + annthurium + 4 hours ago +
+
mind adding the missing semicolon here?
+
+ +
+ + + +
+
+
+ +
+
+ +
+ + + lib/controllers/ + pr-reviews-controller.js + 22 + + 4h + + + 
+                
{ '      ),' }

+                
{ '    }),' }

+                
{ '    getBufferRowForDiffPosition: PropTypes.func.isRequired,' }

+                
{ '    isPatchTooLargeOrCollapsed: PropTypes.func.isRequired,' }

+
+
+
+
+ + annthurium + 4 hours ago +
+
this is a really long name -- can you come up with something more concise?
+
+
+
+ + vanessayuenn + 18 minutes ago +
+
how about just isPatchCollapsed?
+
+ +
+ + + +
+
+
+ +
+
+ +
+ + + lib/models/patch/ + builder.js + 12 + + 1h + + + 
+                
{ 'export const DEFAULT_OPTIONS = {' }

+                
{ '  // Number of lines after which we consider the diff "large"' }

+                
{ '  // TODO: Set this based on performance measurements' }

+                
{ '  largeDiffThreshold: 100,' }

+
+
+
+
+ + annthurium + 1 hour ago +
+
@simurai: how many lines do you think constitutes a large diff? Not just from a performance perspective, but from a user experience perspective. Like how many lines is disruptive to a user when they're trying to read, because often large diffs are the result of auto generated code.
+
+
+
+ + simurai + 1 hour ago +
+
Hmmm.. will large diffs be collapsed by default or there is a "load" button? Maybe if the diff is so large that it fills the whole scroll height. Then I can uncollapse only if I'm really interested in that file. 100 seems fine. 👍
+
+
+
+ + annthurium + 1 hour ago +
+ +
+
+
+ + vanessayuenn + 25 minutes ago +
+
mm we were using 1000 lines as the threshold before, but that's for performance reasons. 100 does seem a bit small though considering it's counting both deleted and added lines. 🤔
+
+ 300 seems a bit more reasonable, but still an arbitrary number.
+
+ +
+ + + +
+
+
+ +
+
+ +
+ + + lib/models/patch/ + multi-file-patch.js + 359 + + 4h + + + 
+                
{ '    }' }

+                
{ '  }' }

+                
{ '' }

+                
{ '  isPatchTooLargeOrCollapsed = filePatchPath => {' }

+
+
+
+
+ + annthurium + 4 hours ago +
+
same as above - this name is too long and the line length linters are gonna be unhappy.
+
+ +
+ + + +
+
+
+ +
+
+ +
+ + + lib/views/ + file-patch-header-view.js + 52 + + 2h + + + 
+                
{ '      
' }

+                
{ '        ' }

+                
{ '          {this.renderTitle()}' }

+                
{ '          {this.renderCollapseButton()}' }

+
+
+
+
+ + simurai + 2 hours ago +
+
Should we move the chevron icon to the left? So that the position doesn't jump when there are more buttons. Alternative would be to move it all the way to the right.
+
+ +
+ + + +
+
+
+ +
+
+ +
+ + + test/views/ + pr-comments-view.test.js + 17 + + 4h + + + 
+                
{ '    };' }

+                
{ '    return shallow(' }

+                
{ '      
+                
{ '        isPatchTooLargeOrCollapsed={sinon.stub().returns(false)}' }

+
+
+
+
+ + annthurium + 4 hours ago +
+
again, this variable name is too long
+
+ +
+ + + +
+
+
+ +
+
+ +
+ + + lib/models/patch/ + builder.js + 280 + + 4h + + + 
+                
{ '  /*' }

+                
{ '   * Construct a String containing diagnostic information about the internal state of this FilePatch.' }

+                
{ '   */' }

+                
{ '  inspect(opts = {}) {' }

+
+
+
+
+ + annthurium + 4 hours ago +
+
nice! This is going to be so useful when we are trying to debug the marker layer on a text buffer.
+
+ +
+ + + +
+
+
+ annthurium marked this as resolved + +
+
+ +
+
+
); } diff --git a/styles/review.less b/styles/review.less new file mode 100644 index 0000000000..3730283ef5 --- /dev/null +++ b/styles/review.less @@ -0,0 +1,388 @@ +@import "variables"; + +@avatar-size: 16px; +@font-size-default: @font-size; +@font-size-body: @font-size + 1px; +@nav-size: 2.5em; +@background-color: mix(@base-background-color, @tool-panel-background-color, 66%); + + +// PR ------------------------------------ + +.github-Pr { + flex: 1; + overflow-x: auto; +} + + +// Reviews ---------------------------------------------- + +.github-Reviews { + flex: 1; + overflow-y: auto; + + &-section { + border-bottom: 1px solid @base-border-color; + } + + &-header { + display: flex; + align-items: center; + padding: @component-padding; + cursor: default; + } + + &-title { + flex: 1; + font-size: 1.15em; + margin: 0; + } + + &-count { + color: @text-color-subtle; + } + + &-countNr { + color: @text-color-highlight; + } + + &-progessBar { + width: 5em; + margin-left: @component-padding; + } + + &-container { + font-size: @font-size-default; + padding: @component-padding; + padding-top: 0; + + &.is-resolved { + padding-top: @component-padding; + border-top: 1px solid @base-border-color; + } + } +} + + +// Review Summaries ------------------------------------------ + +.github-ReviewSummary { + padding: @component-padding; + border: 1px solid @base-border-color; + border-radius: @component-border-radius; + background-color: @background-color; + + & + & { + margin-top: @component-padding; + } + + &-header { + display: flex; + } + + &-icon { + vertical-align: -3px; + &.icon-check { + color: @text-color-success; + } + &.icon-alert { + color: @text-color-warning; + } + &.icon-comment { + color: @text-color-subtle; + } + } + + &-avatar { + margin-right: .5em; + width: @avatar-size; + height: @avatar-size; + border-radius: @component-border-radius; + image-rendering: -webkit-optimize-contrast; + } + + &-username { + margin-right: .3em; + font-weight: 500; + } + + &-type { + color: @text-color-subtle; + } + + &-timeAgo { + margin-left: auto; + color: @text-color-subtle; + } + + &-comment { + position: relative; + margin-top: @component-padding/2; + margin-left: 7px; + padding-left: 12px; + font-size: @font-size-body; + border-left: 2px solid @base-border-color; + } +} + + +// Review Comments ---------------------------------------------- + +.github-Review { + position: relative; + border: 1px solid @base-border-color; + border-radius: @component-border-radius; + background-color: @background-color; + + & + & { + margin-top: @component-padding; + } + + // Header ------------------ + + &-reference { + display: flex; + align-items: center; + padding-left: @component-padding; + height: @nav-size; + cursor: default; + + &::-webkit-details-marker { + margin-right: @component-padding/1.5; + } + + .is-resolved & { + &::-webkit-details-marker { + display: none; + } + } + } + + &-resolvedIcon { + display: none; + + .is-resolved & { + display: inline-block; + } + + &:before { + font-size: 12px; + width: 15px; // magic to match the triangle + margin-right: 0; + } + } + + &-path { + color: @text-color-subtle; + text-overflow: ellipsis; + white-space: nowrap; + overflow: hidden; + pointer-events: none; + } + + &-file { + color: @text-color; + margin-right: @component-padding/2; + font-weight: 500; + text-overflow: ellipsis; + white-space: nowrap; + overflow: hidden; + pointer-events: none; + } + + &-lineNr { + color: @text-color-subtle; + margin-right: @component-padding/2; + } + + &-referenceAvatar { + margin-left: auto; + margin-right: @component-padding/2; + width: @avatar-size; + height: @avatar-size; + border-radius: @component-border-radius; + image-rendering: -webkit-optimize-contrast; + + .github-Review[open] & { + display: none; + } + } + + &-referenceTimeAgo { + color: @text-color-subtle; + margin-right: @component-padding/2; + .github-Review[open] & { + display: none; + } + } + + &-nav { + display: none; + margin-left: auto; + .github-Review[open] & { + display: initial; + } + } + &-navButton { + width: @nav-size; + height: @nav-size; + padding: 0; + border: none; + border-left: 1px solid @base-border-color; + background-color: transparent; + cursor: default; + + &:hover { + background-color: @background-color-highlight; + } + &:active { + background-color: @background-color-selected; + } + + &:before { + width: auto; + margin-right: 0; + } + } + + + // Diff ------------------ + + &-diff { + padding: 0; + color: @text-color; + font-family: var(--editor-font-family); + border-top: 1px solid @base-border-color; + border-bottom: 1px solid @base-border-color; + border-radius: 0; + background: @base-background-color; + } + + &-diffLine { + padding: 0 @component-padding/2; + line-height: 1.75; + + &:last-child { + font-weight: 500; + } + + &::before { + content: " "; + margin-right: .5em; + } + + &.is-added { + color: mix(@text-color-success, @text-color-highlight, 25%); + background-color: mix(@background-color-success, @base-background-color, 20%); + &::before { + content: "+"; + } + } + &.is-deleted { + color: mix(@text-color-error, @text-color-highlight, 25%); + background-color: mix(@background-color-error, @base-background-color, 20%); + &::before { + content: "-"; + } + } + } + + &-diffLineIcon { + margin-right: .5em; + } + + + // Comments ------------------ + + &-comments { + padding: @component-padding; + padding-left: @component-padding + @avatar-size + 5px; // space for avatar + } + + &-comment { + margin-bottom: @component-padding*1.5; + } + + &-header { + display: flex; + align-items: center; + margin-bottom: @component-padding/4; + } + + &-avatar { + position: absolute; + left: @component-padding; + margin-right: .5em; + width: @avatar-size; + height: @avatar-size; + border-radius: @component-border-radius; + image-rendering: -webkit-optimize-contrast; + } + + &-username { + margin-right: .5em; + font-weight: 500; + } + + &-timeAgo { + color: @text-color-subtle; + } + + &-text { + font-size: @font-size-body; + + a { + font-weight: 500; + } + } + + &-reply { + margin-top: @component-padding; + } + + &-replyInput { + width: 100%; + height: 2.125em; // one line + min-height: 2.125em; + resize: vertical; + + &:focus { + height: 3.75em; // two lines + } + + &:focus + .github-ReviewComment-replyButton { + display: block; + } + } + + &-replyButton { + display: none; + margin-top: @component-padding; + } + + + // Footer ------------------ + + &-footer { + display: flex; + align-items: center; + justify-content: space-between; + padding: @component-padding; + border-top: 1px solid @base-border-color; + } + + &-resolveText { + margin-right: @component-padding; + color: @text-color-subtle; + } + + &-resolveButton { + margin-left: auto; + &.btn.icon { + &:before { + font-size: 14px; + } + } + } + +} From 4ab61052e0a7bff6b7cfa4fe71c20c0369a93582 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Mon, 4 Mar 2019 16:36:59 +0100 Subject: [PATCH 027/738] make it more programmatic --- lib/views/reviews-view.js | 695 ++++++++++++++++++-------------------- 1 file changed, 337 insertions(+), 358 deletions(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 309581d925..22a6bc425e 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -2,21 +2,19 @@ import React from 'react'; import {inspect} from 'util'; export default class ReviewsView extends React.Component { - render() { - return ( -
- - {/* Review Summaries ------------------------------------------ */} - -
- -

Reviews

- -
+ renderReviewSummaries() { + return ( +
+ +

Reviews

+ +
+ {[1,2].map(({something}) => (
- + {/*icon-check: approved, icon-comment: commented, icon-alert: requested changes */} + vanessayuenn approved these changes @@ -26,375 +24,356 @@ export default class ReviewsView extends React.Component { looks good! 🚢
+ ))} + + + ); + } -
-
- - - simurai - requested changes - 2 hours ago -
-
- This is a great addition. Before merging we might should rethink the position of the icon. -
-
+ renderReviewComments() { + return ( +
+ +

Review comments

+ + Resolved 1 of 7 + + + +
-
-
- - - annthurium - commented - 4 hours ago -
-
- looking good so far -- just found a few nits to address. +
+ + + lib/controllers/ + commit-detail-controller.js + 19 + + 4h + + + 
+              
{ '      messageCollapsible: this.props.commit.isBodyLong(),' }

+              
{ '      messageOpen: !this.props.commit.isBodyLong(),' }

+              
{ '    };' }

+              
{ '    }' }

+
+
+
+
+ + annthurium + 4 hours ago +
+
mind adding the missing semicolon here?
-
- -
-
- +
+ + + +
+ +
+ +
+ - {/* Review Comments ------------------------------------------ */} - -
- -

Review comments

- - Resolved 1 of 7 - - - -
- -
- - - lib/controllers/ - commit-detail-controller.js - 19 - - 4h - - - 
-                
{ '      messageCollapsible: this.props.commit.isBodyLong(),' }

-                
{ '      messageOpen: !this.props.commit.isBodyLong(),' }

-                
{ '    };' }

-                
{ '    }' }

-
-
-
-
- - annthurium - 4 hours ago -
-
mind adding the missing semicolon here?
-
- -
+
+ + + lib/controllers/ + pr-reviews-controller.js + 22 + + 4h + + + 
+              
{ '      ),' }

+              
{ '    }),' }

+              
{ '    getBufferRowForDiffPosition: PropTypes.func.isRequired,' }

+              
{ '    isPatchTooLargeOrCollapsed: PropTypes.func.isRequired,' }

+
+
+
+
- - -
-
-
- -
-
+ annthurium + 4 hours ago + +
this is a really long name -- can you come up with something more concise?
+
+
+
+ + vanessayuenn + 18 minutes ago +
+
how about just isPatchCollapsed?
+
-
- - - lib/controllers/ - pr-reviews-controller.js - 22 - - 4h - - - 
-                
{ '      ),' }

-                
{ '    }),' }

-                
{ '    getBufferRowForDiffPosition: PropTypes.func.isRequired,' }

-                
{ '    isPatchTooLargeOrCollapsed: PropTypes.func.isRequired,' }

-
-
-
-
- - annthurium - 4 hours ago -
-
this is a really long name -- can you come up with something more concise?
-
-
-
- - vanessayuenn - 18 minutes ago -
-
how about just isPatchCollapsed?
-
+
+ + + +
+
+
+ +
+
-
+
+ + + lib/models/patch/ + builder.js + 12 + + 1h + + + 
+              
{ 'export const DEFAULT_OPTIONS = {' }

+              
{ '  // Number of lines after which we consider the diff "large"' }

+              
{ '  // TODO: Set this based on performance measurements' }

+              
{ '  largeDiffThreshold: 100,' }

+
+
+
+
- - -
-
-
- -
-
- -
- - - lib/models/patch/ - builder.js - 12 - - 1h - - - 
-                
{ 'export const DEFAULT_OPTIONS = {' }

-                
{ '  // Number of lines after which we consider the diff "large"' }

-                
{ '  // TODO: Set this based on performance measurements' }

-                
{ '  largeDiffThreshold: 100,' }

-
-
-
-
- - annthurium - 1 hour ago -
-
@simurai: how many lines do you think constitutes a large diff? Not just from a performance perspective, but from a user experience perspective. Like how many lines is disruptive to a user when they're trying to read, because often large diffs are the result of auto generated code.
-
-
-
- - simurai - 1 hour ago -
-
Hmmm.. will large diffs be collapsed by default or there is a "load" button? Maybe if the diff is so large that it fills the whole scroll height. Then I can uncollapse only if I'm really interested in that file. 100 seems fine. 👍
-
-
-
- - annthurium - 1 hour ago -
- -
-
-
- - vanessayuenn - 25 minutes ago -
-
mm we were using 1000 lines as the threshold before, but that's for performance reasons. 100 does seem a bit small though considering it's counting both deleted and added lines. 🤔
-
- 300 seems a bit more reasonable, but still an arbitrary number.
-
- -
+ annthurium + 1 hour ago + +
@simurai: how many lines do you think constitutes a large diff? Not just from a performance perspective, but from a user experience perspective. Like how many lines is disruptive to a user when they're trying to read, because often large diffs are the result of auto generated code.
+
+
+
+ + simurai + 1 hour ago +
+
Hmmm.. will large diffs be collapsed by default or there is a "load" button? Maybe if the diff is so large that it fills the whole scroll height. Then I can uncollapse only if I'm really interested in that file. 100 seems fine. 👍
+
+
+
- - -
-
-
- -
-
+ annthurium + 1 hour ago + + +
+
+
+ + vanessayuenn + 25 minutes ago +
+
mm we were using 1000 lines as the threshold before, but that's for performance reasons. 100 does seem a bit small though considering it's counting both deleted and added lines. 🤔
+
+ 300 seems a bit more reasonable, but still an arbitrary number.
+
-
- - - lib/models/patch/ - multi-file-patch.js - 359 - - 4h - - - 
-                
{ '    }' }

-                
{ '  }' }

-                
{ '' }

-                
{ '  isPatchTooLargeOrCollapsed = filePatchPath => {' }

-
-
-
-
- - annthurium - 4 hours ago -
-
same as above - this name is too long and the line length linters are gonna be unhappy.
-
+
+ + + +
+
+
+ +
+
-
+
+ + + lib/models/patch/ + multi-file-patch.js + 359 + + 4h + + + 
+              
{ '    }' }

+              
{ '  }' }

+              
{ '' }

+              
{ '  isPatchTooLargeOrCollapsed = filePatchPath => {' }

+
+
+
+
- - -
-
-
- -
-
+ annthurium + 4 hours ago + +
same as above - this name is too long and the line length linters are gonna be unhappy.
+
-
- - - lib/views/ - file-patch-header-view.js - 52 - - 2h - - - 
-                
{ '      
' }

-                
{ '        ' }

-                
{ '          {this.renderTitle()}' }

-                
{ '          {this.renderCollapseButton()}' }

-
-
-
-
- - simurai - 2 hours ago -
-
Should we move the chevron icon to the left? So that the position doesn't jump when there are more buttons. Alternative would be to move it all the way to the right.
-
+
+ + + +
+
+
+ +
+
-
- - - -
-
-
- -
-
+
+ + + lib/views/ + file-patch-header-view.js + 52 + + 2h + + + 
+              
{ '      
' }

+              
{ '        ' }

+              
{ '          {this.renderTitle()}' }

+              
{ '          {this.renderCollapseButton()}' }

+
+
+
+
+ + simurai + 2 hours ago +
+
Should we move the chevron icon to the left? So that the position doesn't jump when there are more buttons. Alternative would be to move it all the way to the right.
+
-
- - - test/views/ - pr-comments-view.test.js - 17 - - 4h - - - 
-                
{ '    };' }

-                
{ '    return shallow(' }

-                
{ '      
-                
{ '        isPatchTooLargeOrCollapsed={sinon.stub().returns(false)}' }

-
-
-
-
- - annthurium - 4 hours ago -
-
again, this variable name is too long
-
+
+ + + +
+
+
+ +
+
-
+
+ + + test/views/ + pr-comments-view.test.js + 17 + + 4h + + + 
+              
{ '    };' }

+              
{ '    return shallow(' }

+              
{ '      
+              
{ '        isPatchTooLargeOrCollapsed={sinon.stub().returns(false)}' }

+
+
+
+
- - -
-
-
- -
-
+ annthurium + 4 hours ago + +
again, this variable name is too long
+
-
- - - lib/models/patch/ - builder.js - 280 - - 4h - - - 
-                
{ '  /*' }

-                
{ '   * Construct a String containing diagnostic information about the internal state of this FilePatch.' }

-                
{ '   */' }

-                
{ '  inspect(opts = {}) {' }

-
-
-
-
- - annthurium - 4 hours ago -
-
nice! This is going to be so useful when we are trying to debug the marker layer on a text buffer.
-
+
+ + + +
+
+
+ +
+
-
+
+ + + lib/models/patch/ + builder.js + 280 + + 4h + + + 
+              
{ '  /*' }

+              
{ '   * Construct a String containing diagnostic information about the internal state of this FilePatch.' }

+              
{ '   */' }

+              
{ '  inspect(opts = {}) {' }

+
+
+
+
- - -
-
-
- annthurium marked this as resolved - -
-
+ annthurium + 4 hours ago + +
nice! This is going to be so useful when we are trying to debug the marker layer on a text buffer.
+
+ +
+ + + +
+
+
+ annthurium marked this as resolved + +
+
-
-
+ + + ) + } + render() { + return ( +
+ {this.renderReviewSummaries()} + {this.renderReviewComments()}
); } From 0491894267add6fa74d633074f71378bf8c379a8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 10:45:37 -0500 Subject: [PATCH 028/738] Render the prReviewsContainer fragment in ReviewsContainer --- .../reviewsContainerQuery.graphql.js | 579 ++++++++++++++++-- lib/containers/reviews-container.js | 18 +- 2 files changed, 557 insertions(+), 40 deletions(-) diff --git a/lib/containers/__generated__/reviewsContainerQuery.graphql.js b/lib/containers/__generated__/reviewsContainerQuery.graphql.js index df5eb0e96a..1b17c91da2 100644 --- a/lib/containers/__generated__/reviewsContainerQuery.graphql.js +++ b/lib/containers/__generated__/reviewsContainerQuery.graphql.js @@ -1,6 +1,6 @@ /** * @flow - * @relayHash 68140a8082e21c7d56516fb6ca6c8db7 + * @relayHash 2ff5fd99b183d2c9a7e114fa51f38d04 */ /* eslint-disable */ @@ -9,13 +9,22 @@ /*:: import type { ConcreteRequest } from 'relay-runtime'; +type prReviewsContainer_pullRequest$ref = any; export type reviewsContainerQueryVariables = {| repoOwner: string, repoName: string, + prNumber: number, + reviewCount: number, + reviewCursor?: ?string, + commentCount: number, + commentCursor?: ?string, |}; export type reviewsContainerQueryResponse = {| +repository: ?{| - +id: string + +pullRequest: ?{| + +$fragmentRefs: prReviewsContainer_pullRequest$ref + |}, + +id: string, |} |}; export type reviewsContainerQuery = {| @@ -29,11 +38,94 @@ export type reviewsContainerQuery = {| query reviewsContainerQuery( $repoOwner: String! $repoName: String! + $prNumber: Int! + $reviewCount: Int! + $reviewCursor: String + $commentCount: Int! + $commentCursor: String ) { repository(owner: $repoOwner, name: $repoName) { + pullRequest(number: $prNumber) { + ...prReviewsContainer_pullRequest_y4qc0 + id + } id } } + +fragment prReviewsContainer_pullRequest_y4qc0 on PullRequest { + url + reviews(first: $reviewCount, after: $reviewCursor) { + pageInfo { + hasNextPage + endCursor + } + edges { + cursor + node { + id + body + commitId: commit { + oid + id + } + state + submittedAt + login: author { + __typename + login + ... on Node { + id + } + } + author { + __typename + avatarUrl + ... on Node { + id + } + } + ...prReviewCommentsContainer_review_1VbUmL + __typename + } + } + } +} + +fragment prReviewCommentsContainer_review_1VbUmL on PullRequestReview { + id + submittedAt + comments(first: $commentCount, after: $commentCursor) { + pageInfo { + hasNextPage + endCursor + } + edges { + cursor + node { + id + author { + __typename + avatarUrl + login + ... on Node { + id + } + } + bodyHTML + isMinimized + path + position + replyTo { + id + } + createdAt + url + __typename + } + } + } +} */ const node/*: ConcreteRequest*/ = (function(){ @@ -49,39 +141,153 @@ var v0 = [ "name": "repoName", "type": "String!", "defaultValue": null + }, + { + "kind": "LocalArgument", + "name": "prNumber", + "type": "Int!", + "defaultValue": null + }, + { + "kind": "LocalArgument", + "name": "reviewCount", + "type": "Int!", + "defaultValue": null + }, + { + "kind": "LocalArgument", + "name": "reviewCursor", + "type": "String", + "defaultValue": null + }, + { + "kind": "LocalArgument", + "name": "commentCount", + "type": "Int!", + "defaultValue": null + }, + { + "kind": "LocalArgument", + "name": "commentCursor", + "type": "String", + "defaultValue": null } ], v1 = [ { - "kind": "LinkedField", - "alias": null, - "name": "repository", - "storageKey": null, - "args": [ - { - "kind": "Variable", - "name": "name", - "variableName": "repoName", - "type": "String!" - }, - { - "kind": "Variable", - "name": "owner", - "variableName": "repoOwner", - "type": "String!" - } - ], - "concreteType": "Repository", - "plural": false, - "selections": [ - { - "kind": "ScalarField", - "alias": null, - "name": "id", - "args": null, - "storageKey": null - } - ] + "kind": "Variable", + "name": "name", + "variableName": "repoName", + "type": "String!" + }, + { + "kind": "Variable", + "name": "owner", + "variableName": "repoOwner", + "type": "String!" + } +], +v2 = [ + { + "kind": "Variable", + "name": "number", + "variableName": "prNumber", + "type": "Int!" + } +], +v3 = { + "kind": "ScalarField", + "alias": null, + "name": "id", + "args": null, + "storageKey": null +}, +v4 = { + "kind": "ScalarField", + "alias": null, + "name": "url", + "args": null, + "storageKey": null +}, +v5 = [ + { + "kind": "Variable", + "name": "after", + "variableName": "reviewCursor", + "type": "String" + }, + { + "kind": "Variable", + "name": "first", + "variableName": "reviewCount", + "type": "Int" + } +], +v6 = { + "kind": "LinkedField", + "alias": null, + "name": "pageInfo", + "storageKey": null, + "args": null, + "concreteType": "PageInfo", + "plural": false, + "selections": [ + { + "kind": "ScalarField", + "alias": null, + "name": "hasNextPage", + "args": null, + "storageKey": null + }, + { + "kind": "ScalarField", + "alias": null, + "name": "endCursor", + "args": null, + "storageKey": null + } + ] +}, +v7 = { + "kind": "ScalarField", + "alias": null, + "name": "cursor", + "args": null, + "storageKey": null +}, +v8 = { + "kind": "ScalarField", + "alias": null, + "name": "__typename", + "args": null, + "storageKey": null +}, +v9 = { + "kind": "ScalarField", + "alias": null, + "name": "login", + "args": null, + "storageKey": null +}, +v10 = { + "kind": "ScalarField", + "alias": null, + "name": "avatarUrl", + "args": null, + "storageKey": null +}, +v11 = [ + { + "kind": "Variable", + "name": "after", + "variableName": "commentCursor", + "type": "String" + }, + { + "kind": "Variable", + "name": "first", + "variableName": "commentCount", + "type": "Int" } ]; return { @@ -92,23 +298,326 @@ return { "type": "Query", "metadata": null, "argumentDefinitions": (v0/*: any*/), - "selections": (v1/*: any*/) + "selections": [ + { + "kind": "LinkedField", + "alias": null, + "name": "repository", + "storageKey": null, + "args": (v1/*: any*/), + "concreteType": "Repository", + "plural": false, + "selections": [ + { + "kind": "LinkedField", + "alias": null, + "name": "pullRequest", + "storageKey": null, + "args": (v2/*: any*/), + "concreteType": "PullRequest", + "plural": false, + "selections": [ + { + "kind": "FragmentSpread", + "name": "prReviewsContainer_pullRequest", + "args": [ + { + "kind": "Variable", + "name": "commentCount", + "variableName": "commentCount", + "type": null + }, + { + "kind": "Variable", + "name": "commentCursor", + "variableName": "commentCursor", + "type": null + }, + { + "kind": "Variable", + "name": "reviewCount", + "variableName": "reviewCount", + "type": null + }, + { + "kind": "Variable", + "name": "reviewCursor", + "variableName": "reviewCursor", + "type": null + } + ] + } + ] + }, + (v3/*: any*/) + ] + } + ] }, "operation": { "kind": "Operation", "name": "reviewsContainerQuery", "argumentDefinitions": (v0/*: any*/), - "selections": (v1/*: any*/) + "selections": [ + { + "kind": "LinkedField", + "alias": null, + "name": "repository", + "storageKey": null, + "args": (v1/*: any*/), + "concreteType": "Repository", + "plural": false, + "selections": [ + { + "kind": "LinkedField", + "alias": null, + "name": "pullRequest", + "storageKey": null, + "args": (v2/*: any*/), + "concreteType": "PullRequest", + "plural": false, + "selections": [ + (v4/*: any*/), + { + "kind": "LinkedField", + "alias": null, + "name": "reviews", + "storageKey": null, + "args": (v5/*: any*/), + "concreteType": "PullRequestReviewConnection", + "plural": false, + "selections": [ + (v6/*: any*/), + { + "kind": "LinkedField", + "alias": null, + "name": "edges", + "storageKey": null, + "args": null, + "concreteType": "PullRequestReviewEdge", + "plural": true, + "selections": [ + (v7/*: any*/), + { + "kind": "LinkedField", + "alias": null, + "name": "node", + "storageKey": null, + "args": null, + "concreteType": "PullRequestReview", + "plural": false, + "selections": [ + (v3/*: any*/), + { + "kind": "ScalarField", + "alias": null, + "name": "body", + "args": null, + "storageKey": null + }, + { + "kind": "LinkedField", + "alias": "commitId", + "name": "commit", + "storageKey": null, + "args": null, + "concreteType": "Commit", + "plural": false, + "selections": [ + { + "kind": "ScalarField", + "alias": null, + "name": "oid", + "args": null, + "storageKey": null + }, + (v3/*: any*/) + ] + }, + { + "kind": "ScalarField", + "alias": null, + "name": "state", + "args": null, + "storageKey": null + }, + { + "kind": "ScalarField", + "alias": null, + "name": "submittedAt", + "args": null, + "storageKey": null + }, + { + "kind": "LinkedField", + "alias": "login", + "name": "author", + "storageKey": null, + "args": null, + "concreteType": null, + "plural": false, + "selections": [ + (v8/*: any*/), + (v9/*: any*/), + (v3/*: any*/) + ] + }, + { + "kind": "LinkedField", + "alias": null, + "name": "author", + "storageKey": null, + "args": null, + "concreteType": null, + "plural": false, + "selections": [ + (v8/*: any*/), + (v10/*: any*/), + (v3/*: any*/) + ] + }, + { + "kind": "LinkedField", + "alias": null, + "name": "comments", + "storageKey": null, + "args": (v11/*: any*/), + "concreteType": "PullRequestReviewCommentConnection", + "plural": false, + "selections": [ + (v6/*: any*/), + { + "kind": "LinkedField", + "alias": null, + "name": "edges", + "storageKey": null, + "args": null, + "concreteType": "PullRequestReviewCommentEdge", + "plural": true, + "selections": [ + (v7/*: any*/), + { + "kind": "LinkedField", + "alias": null, + "name": "node", + "storageKey": null, + "args": null, + "concreteType": "PullRequestReviewComment", + "plural": false, + "selections": [ + (v3/*: any*/), + { + "kind": "LinkedField", + "alias": null, + "name": "author", + "storageKey": null, + "args": null, + "concreteType": null, + "plural": false, + "selections": [ + (v8/*: any*/), + (v10/*: any*/), + (v9/*: any*/), + (v3/*: any*/) + ] + }, + { + "kind": "ScalarField", + "alias": null, + "name": "bodyHTML", + "args": null, + "storageKey": null + }, + { + "kind": "ScalarField", + "alias": null, + "name": "isMinimized", + "args": null, + "storageKey": null + }, + { + "kind": "ScalarField", + "alias": null, + "name": "path", + "args": null, + "storageKey": null + }, + { + "kind": "ScalarField", + "alias": null, + "name": "position", + "args": null, + "storageKey": null + }, + { + "kind": "LinkedField", + "alias": null, + "name": "replyTo", + "storageKey": null, + "args": null, + "concreteType": "PullRequestReviewComment", + "plural": false, + "selections": [ + (v3/*: any*/) + ] + }, + { + "kind": "ScalarField", + "alias": null, + "name": "createdAt", + "args": null, + "storageKey": null + }, + (v4/*: any*/), + (v8/*: any*/) + ] + } + ] + } + ] + }, + { + "kind": "LinkedHandle", + "alias": null, + "name": "comments", + "args": (v11/*: any*/), + "handle": "connection", + "key": "PrReviewCommentsContainer_comments", + "filters": null + }, + (v8/*: any*/) + ] + } + ] + } + ] + }, + { + "kind": "LinkedHandle", + "alias": null, + "name": "reviews", + "args": (v5/*: any*/), + "handle": "connection", + "key": "PrReviewsContainer_reviews", + "filters": null + }, + (v3/*: any*/) + ] + }, + (v3/*: any*/) + ] + } + ] }, "params": { "operationKind": "query", "name": "reviewsContainerQuery", "id": null, - "text": "query reviewsContainerQuery(\n $repoOwner: String!\n $repoName: String!\n) {\n repository(owner: $repoOwner, name: $repoName) {\n id\n }\n}\n", + "text": "query reviewsContainerQuery(\n $repoOwner: String!\n $repoName: String!\n $prNumber: Int!\n $reviewCount: Int!\n $reviewCursor: String\n $commentCount: Int!\n $commentCursor: String\n) {\n repository(owner: $repoOwner, name: $repoName) {\n pullRequest(number: $prNumber) {\n ...prReviewsContainer_pullRequest_y4qc0\n id\n }\n id\n }\n}\n\nfragment prReviewsContainer_pullRequest_y4qc0 on PullRequest {\n url\n reviews(first: $reviewCount, after: $reviewCursor) {\n pageInfo {\n hasNextPage\n endCursor\n }\n edges {\n cursor\n node {\n id\n body\n commitId: commit {\n oid\n id\n }\n state\n submittedAt\n login: author {\n __typename\n login\n ... on Node {\n id\n }\n }\n author {\n __typename\n avatarUrl\n ... on Node {\n id\n }\n }\n ...prReviewCommentsContainer_review_1VbUmL\n __typename\n }\n }\n }\n}\n\nfragment prReviewCommentsContainer_review_1VbUmL on PullRequestReview {\n id\n submittedAt\n comments(first: $commentCount, after: $commentCursor) {\n pageInfo {\n hasNextPage\n endCursor\n }\n edges {\n cursor\n node {\n id\n author {\n __typename\n avatarUrl\n login\n ... on Node {\n id\n }\n }\n bodyHTML\n isMinimized\n path\n position\n replyTo {\n id\n }\n createdAt\n url\n __typename\n }\n }\n }\n}\n", "metadata": {} } }; })(); // prettier-ignore -(node/*: any*/).hash = '2a432f9e2a062215c96118ec0b5070cd'; +(node/*: any*/).hash = '51bf09a7273a2f166d4ff1c89b433cf6'; module.exports = node; diff --git a/lib/containers/reviews-container.js b/lib/containers/reviews-container.js index 81cd707a27..79a78d857e 100644 --- a/lib/containers/reviews-container.js +++ b/lib/containers/reviews-container.js @@ -74,13 +74,21 @@ export default class ReviewsContainer extends React.Component { ( $repoOwner: String! $repoName: String! - # $prNumber: Int! - # $reviewCount: Int! - # $reviewCursor: String - # $commentCount: Int! - # $commentCursor: String + $prNumber: Int! + $reviewCount: Int! + $reviewCursor: String + $commentCount: Int! + $commentCursor: String ) { repository(owner: $repoOwner, name: $repoName) { + pullRequest(number: $prNumber) { + ...prReviewsContainer_pullRequest @arguments( + reviewCount: $reviewCount + reviewCursor: $reviewCursor + commentCount: $commentCount + commentCursor: $commentCursor + ) + } id } } From c9d6b998c06a2fbbb8718aaa900585d2bf2a3a3e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 10:53:44 -0500 Subject: [PATCH 029/738] :shirt: lint reviews-view --- lib/views/reviews-view.js | 93 +++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 22a6bc425e..a804f27c8d 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -1,5 +1,4 @@ import React from 'react'; -import {inspect} from 'util'; export default class ReviewsView extends React.Component { @@ -10,12 +9,12 @@ export default class ReviewsView extends React.Component {

Reviews

- {[1,2].map(({something}) => ( + {[1, 2].map(({something}) => (
- {/*icon-check: approved, icon-comment: commented, icon-alert: requested changes */} + {/* icon-check: approved, icon-comment: commented, icon-alert: requested changes */} - + vanessayuenn approved these changes 18 minutes ago @@ -37,7 +36,7 @@ export default class ReviewsView extends React.Component {

Review comments

Resolved 1 of 7 - +
@@ -48,12 +47,12 @@ export default class ReviewsView extends React.Component { lib/controllers/ commit-detail-controller.js 19 - + 4h 
@@ -65,7 +64,7 @@ export default class ReviewsView extends React.Component {
             

               

                 

-                  
+                  
                   annthurium
                   4 hours ago
                 

@@ -73,8 +72,8 @@ export default class ReviewsView extends React.Component {
               

 
               

-                
-                
+                
+                
                 
               

             

@@ -89,12 +88,12 @@ export default class ReviewsView extends React.Component {
               lib/controllers/
               pr-reviews-controller.js
               22
-              
+              
               4h
               
             
             
@@ -106,7 +105,7 @@ export default class ReviewsView extends React.Component {
             

               

                 

-                  
+                  
                   annthurium
                   4 hours ago
                 

@@ -114,7 +113,7 @@ export default class ReviewsView extends React.Component {
               

               

                 

-                  
+                  
                   vanessayuenn
                   18 minutes ago
                 

@@ -122,8 +121,8 @@ export default class ReviewsView extends React.Component {
               

 
               

-                
-                
+                
+                
                 
               

             

@@ -138,12 +137,12 @@ export default class ReviewsView extends React.Component {
               lib/models/patch/
               builder.js
               12
-              
+              
               1h
               
             
             
@@ -155,7 +154,7 @@ export default class ReviewsView extends React.Component {
             

               

                 

-                  
+                  
                   annthurium
                   1 hour ago
                 

@@ -163,7 +162,7 @@ export default class ReviewsView extends React.Component {
               

               

                 

-                  
+                  
                   simurai
                   1 hour ago
                 

@@ -171,7 +170,7 @@ export default class ReviewsView extends React.Component {
               

               

                 

-                  
+                  
                   annthurium
                   1 hour ago
                 

@@ -179,18 +178,18 @@ export default class ReviewsView extends React.Component {
               

               

                 

-                  
+                  
                   vanessayuenn
                   25 minutes ago
                 

-                
mm we were using 1000 lines as the threshold before, but that's for performance reasons. 100 does seem a bit small though considering it's counting both deleted and added lines. 🤔 

-                

+                
mm we were using 1000 lines as the threshold before, but that's for performance reasons. 100 does seem a bit small though considering it's counting both deleted and added lines. 🤔 

+                  

                 300 seems a bit more reasonable, but still an arbitrary number.

               

 
               

-                
-                
+                
+                
                 
               

             

@@ -205,12 +204,12 @@ export default class ReviewsView extends React.Component {
               lib/models/patch/
               multi-file-patch.js
               359
-              
+              
               4h
               
             
             
@@ -222,7 +221,7 @@ export default class ReviewsView extends React.Component {
             

               

                 

-                  
+                  
                   annthurium
                   4 hours ago
                 

@@ -230,8 +229,8 @@ export default class ReviewsView extends React.Component {
               

 
               

-                
-                
+                
+                
                 
               

             

@@ -246,12 +245,12 @@ export default class ReviewsView extends React.Component {
               lib/views/
               file-patch-header-view.js
               52
-              
+              
               2h
               
             
             
@@ -263,7 +262,7 @@ export default class ReviewsView extends React.Component {
             

               

                 

-                  
+                  
                   simurai
                   2 hours ago
                 

@@ -271,8 +270,8 @@ export default class ReviewsView extends React.Component {
               

 
               

-                
-                
+                
+                
                 
               

             

@@ -287,12 +286,12 @@ export default class ReviewsView extends React.Component {
               test/views/
               pr-comments-view.test.js
               17
-              
+              
               4h
               
             
             
@@ -304,7 +303,7 @@ export default class ReviewsView extends React.Component {
             

               

                 

-                  
+                  
                   annthurium
                   4 hours ago
                 

@@ -312,8 +311,8 @@ export default class ReviewsView extends React.Component {
               

 
               

-                
-                
+                
+                
                 
               

             

@@ -328,12 +327,12 @@ export default class ReviewsView extends React.Component {
               lib/models/patch/
               builder.js
               280
-              
+              
               4h
               
             
             
@@ -345,7 +344,7 @@ export default class ReviewsView extends React.Component {
             

               

                 

-                  
+                  
                   annthurium
                   4 hours ago
                 

@@ -353,8 +352,8 @@ export default class ReviewsView extends React.Component {
               

 
               

-                
-                
+                
+                
                 
               

             

@@ -366,7 +365,7 @@ export default class ReviewsView extends React.Component {
- ) + ); } render() { From da9c23948b05fd3227ef1440655cb8a68ad0eff0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 10:54:12 -0500 Subject: [PATCH 030/738] Fetch reviews and comments with a PullRequestReviewsContainer --- lib/views/reviews-view.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index a804f27c8d..57365fd15c 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -1,4 +1,5 @@ import React from 'react'; +import PullRequestReviewsContainer from '../containers/pr-reviews-container'; export default class ReviewsView extends React.Component { @@ -372,7 +373,9 @@ export default class ReviewsView extends React.Component { return (
{this.renderReviewSummaries()} - {this.renderReviewComments()} + + {_commentThread => this.renderReviewComments()} +
); } From 3d6eaff4bed8aaf0ab8b3fcb83cc51fc7b90f74b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 11:24:49 -0500 Subject: [PATCH 031/738] Call a single render prop with review and comment thread collections --- lib/controllers/pr-reviews-controller.js | 10 +-- lib/views/multi-file-patch-view.js | 7 +- .../controllers/pr-reviews-controller.test.js | 68 ++++++++----------- 3 files changed, 35 insertions(+), 50 deletions(-) diff --git a/lib/controllers/pr-reviews-controller.js b/lib/controllers/pr-reviews-controller.js index 94adeed5a5..79811e3f01 100644 --- a/lib/controllers/pr-reviews-controller.js +++ b/lib/controllers/pr-reviews-controller.js @@ -61,6 +61,8 @@ export default class PullRequestReviewsController extends React.Component { return null; } + const reviews = this.props.pullRequest.reviews.edges.map(edge => edge.node); + const commentThreads = Object.keys(this.state).reverse().map(rootCommentId => { return { rootCommentId, @@ -80,13 +82,7 @@ export default class PullRequestReviewsController extends React.Component { return ( {this.renderCommentFetchingContainers()} - {commentThreads.map(commentThread => { - return ( - - {this.props.children(commentThread)} - - ); - })} + {this.props.children({reviews, commentThreads})} ); } diff --git a/lib/views/multi-file-patch-view.js b/lib/views/multi-file-patch-view.js index edc80adb59..761861af54 100644 --- a/lib/views/multi-file-patch-view.js +++ b/lib/views/multi-file-patch-view.js @@ -386,15 +386,16 @@ export default class MultiFilePatchView extends React.Component { // "forceRerender" ensures that the PullRequestCommentsView re-renders each time that the MultiFilePatchView does. // It doesn't re-query for reviews, but it does re-check patch visibility. return ( - - {commentThread => ( + + {({commentThreads}) => commentThreads.map(commentThread => ( - )} + ))} ); } else { diff --git a/test/controllers/pr-reviews-controller.test.js b/test/controllers/pr-reviews-controller.test.js index e90c1d6ca9..7bbf9e779c 100644 --- a/test/controllers/pr-reviews-controller.test.js +++ b/test/controllers/pr-reviews-controller.test.js @@ -1,19 +1,14 @@ import React from 'react'; import {shallow} from 'enzyme'; import {reviewBuilder} from '../builder/pr'; -import {inspect} from 'util'; import PullRequestReviewsController from '../../lib/controllers/pr-reviews-controller'; import PullRequestReviewCommentsContainer from '../../lib/containers/pr-review-comments-container'; import {PAGE_SIZE, PAGINATION_WAIT_TIME_MS} from '../../lib/helpers'; -function SomeView(props) { - return 
{inspect(props, {depth: 3})}
; -} - describe('PullRequestReviewsController', function() { - function buildApp(opts, overrideProps = {}, childFn = SomeView) { + function buildApp(opts, overrideProps = {}) { const o = { relayHasMore: () => false, relayLoadMore: () => {}, @@ -47,15 +42,13 @@ describe('PullRequestReviewsController', function() { isLoading: o.relayIsLoading, }, pullRequest: {reviews}, + children: () => {}, ...overrideProps, }; - return ( - - {childFn} - - ); + return ; } + it('returns null if props.pullRequest is falsy', function() { const wrapper = shallow(buildApp({}, {pullRequest: null})); assert.isNull(wrapper.getElement()); @@ -79,7 +72,7 @@ describe('PullRequestReviewsController', function() { assert.strictEqual(containers.at(1).prop('review').id, review2.id); }); - it('calls the child render prop for each comment thread', function() { + it('calls the render prop with all reviews and all comment threads', function() { const review1 = reviewBuilder() .id(10) .submittedAt('2019-01-01T10:00:00Z') @@ -100,11 +93,19 @@ describe('PullRequestReviewsController', function() { const reviewSpecs = [review1, review2]; const passThroughProp = 'I only exist for the children'; - const wrapper = shallow(buildApp({reviewSpecs}, {}, thread => { - return ; + const wrapper = shallow(buildApp({reviewSpecs}, { + children: ({reviews, commentThreads}) => ( + + ), })); - assert.isFalse(wrapper.exists('ChildComponent')); + assert.strictEqual(wrapper.find('ChildComponent').prop('passThroughProp'), passThroughProp); + assert.deepEqual(wrapper.find('ChildComponent').prop('reviews').map(review => review.id), [10, 20]); + assert.lengthOf(wrapper.find('ChildComponent').prop('threads'), 0); wrapper.find(PullRequestReviewCommentsContainer).at(0).prop('collectComments')({ reviewId: review1.id, @@ -113,12 +114,11 @@ describe('PullRequestReviewsController', function() { fetchingMoreComments: false, }); - assert.lengthOf(wrapper.find('ChildComponent'), 1); - assert.strictEqual(wrapper.find('ChildComponent').at(0).prop('passThroughProp'), passThroughProp); - assert.deepEqual(wrapper.find('ChildComponent').at(0).prop('thread'), { - rootCommentId: '1', - comments: [review1.comments.edges[0].node], - }); + assert.strictEqual(wrapper.find('ChildComponent').prop('passThroughProp'), passThroughProp); + assert.deepEqual(wrapper.find('ChildComponent').prop('reviews').map(review => review.id), [10, 20]); + assert.deepEqual(wrapper.find('ChildComponent').prop('threads'), [ + {rootCommentId: '1', comments: [review1.comments.edges[0].node]}, + ]); wrapper.find(PullRequestReviewCommentsContainer).at(1).prop('collectComments')({ reviewId: review2.id, @@ -127,25 +127,13 @@ describe('PullRequestReviewsController', function() { fetchingMoreComments: false, }); - assert.lengthOf(wrapper.find('ChildComponent'), 3); - - assert.strictEqual(wrapper.find('ChildComponent').at(0).prop('passThroughProp'), passThroughProp); - assert.deepEqual(wrapper.find('ChildComponent').at(0).prop('thread'), { - rootCommentId: '4', - comments: [review2.comments.edges[2].node], - }); - - assert.strictEqual(wrapper.find('ChildComponent').at(1).prop('passThroughProp'), passThroughProp); - assert.deepEqual(wrapper.find('ChildComponent').at(1).prop('thread'), { - rootCommentId: '2', - comments: [review2.comments.edges[0].node], - }); - - assert.strictEqual(wrapper.find('ChildComponent').at(2).prop('passThroughProp'), passThroughProp); - assert.deepEqual(wrapper.find('ChildComponent').at(2).prop('thread'), { - rootCommentId: '1', - comments: [review1.comments.edges[0].node, review2.comments.edges[1].node], - }); + assert.strictEqual(wrapper.find('ChildComponent').prop('passThroughProp'), passThroughProp); + assert.deepEqual(wrapper.find('ChildComponent').prop('reviews').map(review => review.id), [10, 20]); + assert.deepEqual(wrapper.find('ChildComponent').prop('threads'), [ + {rootCommentId: '4', comments: [review2.comments.edges[2].node]}, + {rootCommentId: '2', comments: [review2.comments.edges[0].node]}, + {rootCommentId: '1', comments: [review1.comments.edges[0].node, review2.comments.edges[1].node]}, + ]); }); describe('collectComments', function() { From 4c3211d15045e1a66a94701a4458624c04a54bef Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 4 Mar 2019 11:29:03 -0500 Subject: [PATCH 032/738] Oops, you _do_ need that pullRequest={} prop --- lib/views/multi-file-patch-view.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/views/multi-file-patch-view.js b/lib/views/multi-file-patch-view.js index 761861af54..f18183dfb5 100644 --- a/lib/views/multi-file-patch-view.js +++ b/lib/views/multi-file-patch-view.js @@ -386,7 +386,7 @@ export default class MultiFilePatchView extends React.Component { // "forceRerender" ensures that the PullRequestCommentsView re-renders each time that the MultiFilePatchView does. // It doesn't re-query for reviews, but it does re-check patch visibility. return ( - + {({commentThreads}) => commentThreads.map(commentThread => ( Date: Mon, 4 Mar 2019 17:32:17 +0100 Subject: [PATCH 033/738] split up to components, pull in markdown view and timeago --- lib/views/reviews-view.js | 390 +++++++------------------------------- 1 file changed, 67 insertions(+), 323 deletions(-) diff --git a/lib/views/reviews-view.js b/lib/views/reviews-view.js index 57365fd15c..19e04cf20b 100644 --- a/lib/views/reviews-view.js +++ b/lib/views/reviews-view.js @@ -1,5 +1,7 @@ import React from 'react'; import PullRequestReviewsContainer from '../containers/pr-reviews-container'; +import GithubDotcomMarkdown from './github-dotcom-markdown'; +import Timeago from './timeago'; export default class ReviewsView extends React.Component { @@ -41,329 +43,7 @@ export default class ReviewsView extends React.Component {
- -
- - - lib/controllers/ - commit-detail-controller.js - 19 - - 4h - - - 
-              
{ '      messageCollapsible: this.props.commit.isBodyLong(),' }

-              
{ '      messageOpen: !this.props.commit.isBodyLong(),' }

-              
{ '    };' }

-              
{ '    }' }

-
-
-
-
- - annthurium - 4 hours ago -
-
mind adding the missing semicolon here?
-
- -
- - - -
-
-
- -
-
- -
- - - lib/controllers/ - pr-reviews-controller.js - 22 - - 4h - - - 
-              
{ '      ),' }

-              
{ '    }),' }

-              
{ '    getBufferRowForDiffPosition: PropTypes.func.isRequired,' }

-              
{ '    isPatchTooLargeOrCollapsed: PropTypes.func.isRequired,' }

-
-
-
-
- - annthurium - 4 hours ago -
-
this is a really long name -- can you come up with something more concise?
-
-
-
- - vanessayuenn - 18 minutes ago -
-
how about just isPatchCollapsed?
-
- -
- - - -
-
-
- -
-
- -
- - - lib/models/patch/ - builder.js - 12 - - 1h - - - 
-              
{ 'export const DEFAULT_OPTIONS = {' }

-              
{ '  // Number of lines after which we consider the diff "large"' }

-              
{ '  // TODO: Set this based on performance measurements' }

-              
{ '  largeDiffThreshold: 100,' }

-
-
-
-
- - annthurium - 1 hour ago -
-
@simurai: how many lines do you think constitutes a large diff? Not just from a performance perspective, but from a user experience perspective. Like how many lines is disruptive to a user when they're trying to read, because often large diffs are the result of auto generated code.
-
-
-
- - simurai - 1 hour ago -
-
Hmmm.. will large diffs be collapsed by default or there is a "load" button? Maybe if the diff is so large that it fills the whole scroll height. Then I can uncollapse only if I'm really interested in that file. 100 seems fine. 👍
-
-
-
- - annthurium - 1 hour ago -
- -
-
-
- - vanessayuenn - 25 minutes ago -
-
mm we were using 1000 lines as the threshold before, but that's for performance reasons. 100 does seem a bit small though considering it's counting both deleted and added lines. 🤔
-
- 300 seems a bit more reasonable, but still an arbitrary number.
-
- -
- - - -
-
-
- -
-
- -
- - - lib/models/patch/ - multi-file-patch.js - 359 - - 4h - - - 
-              
{ '    }' }

-              
{ '  }' }

-              
{ '' }

-              
{ '  isPatchTooLargeOrCollapsed = filePatchPath => {' }

-
-
-
-
- - annthurium - 4 hours ago -
-
same as above - this name is too long and the line length linters are gonna be unhappy.
-
- -
- - - -
-
-
- -
-
- -
- - - lib/views/ - file-patch-header-view.js - 52 - - 2h - - - 
-              
{ '      
' }

-              
{ '        ' }

-              
{ '          {this.renderTitle()}' }

-              
{ '          {this.renderCollapseButton()}' }

-
-
-
-
- - simurai - 2 hours ago -
-
Should we move the chevron icon to the left? So that the position doesn't jump when there are more buttons. Alternative would be to move it all the way to the right.
-
- -
- - - -
-
-
- -
-
- -
- - - test/views/ - pr-comments-view.test.js - 17 - - 4h - - - 
-              
{ '    };' }

-              
{ '    return shallow(' }

-              
{ '      
-              
{ '        isPatchTooLargeOrCollapsed={sinon.stub().returns(false)}' }

-
-
-
-
- - annthurium - 4 hours ago -
-
again, this variable name is too long
-
- -
- - - -
-
-
- -
-
- -
- - - lib/models/patch/ - builder.js - 280 - - 4h - - - 
-              
{ '  /*' }

-              
{ '   * Construct a String containing diagnostic information about the internal state of this FilePatch.' }

-              
{ '   */' }

-              
{ '  inspect(opts = {}) {' }

-
-
-
-
- - annthurium - 4 hours ago -
-
nice! This is going to be so useful when we are trying to debug the marker layer on a text buffer.
-
- -
- - - -
-
-
- annthurium marked this as resolved - -
-
- +
); @@ -373,6 +53,7 @@ export default class ReviewsView extends React.Component { return (
{this.renderReviewSummaries()} + {this.renderReviewComments()} {_commentThread => this.renderReviewComments()} @@ -380,3 +61,66 @@ export default class ReviewsView extends React.Component { ); } } + +class ReviewCommentThreadView extends React.Component { + render() { + const {comments} = this.props.commentThread; + const rootComment = comments[0]; + + return ( +
+ + + lib/controllers/ + commit-detail-controller.js + 19 + {rootComment.author.login} + {rootComment.createdAt} + + + 
+          
{ '      messageCollapsible: this.props.commit.isBodyLong(),' }

+          
{ '      messageOpen: !this.props.commit.isBodyLong(),' }

+          
{ '    };' }

+          
{ '    }' }

+
+ +
+ {comments.map(comment => ( +
+
+ {comment.author.login} + {comment.author.login} + + + +
+
+ +
+
+ ))} + +
+