From 08539125325a127fbbfe379fb04f9115f214365b Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 6 Dec 2018 16:12:31 -0800 Subject: [PATCH 001/102] add tab for changed files --- lib/views/pr-detail-view.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 14caab409b..aa8c36b9d8 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -134,8 +134,14 @@ export class BarePullRequestDetailView extends React.Component { /> Commits + + + Files Changed + - {/* 'Files Changed' and 'Reviews' tabs to be added in the future. */} + {/* 'Reviews' tab to be added in the future. */} {/* overview */} @@ -164,6 +170,10 @@ export class BarePullRequestDetailView extends React.Component { + + {/* files changed */} + + ); } From 66e485c715864cebf8f0bd4220df3ba4271b99c1 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Fri, 7 Dec 2018 16:38:23 -0800 Subject: [PATCH 002/102] Making fetch happen It finally happened! Also made the React component hierarchy and shit. --- lib/containers/pr-changed-files-container.js | 57 +++++++++++++++++++ .../pr-changed-files-controller.js | 11 ++++ lib/views/pr-changed-files-view.js | 9 +++ lib/views/pr-detail-view.js | 8 ++- 4 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 lib/containers/pr-changed-files-container.js create mode 100644 lib/controllers/pr-changed-files-controller.js create mode 100644 lib/views/pr-changed-files-view.js diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js new file mode 100644 index 0000000000..a9af2de70f --- /dev/null +++ b/lib/containers/pr-changed-files-container.js @@ -0,0 +1,57 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +import PullRequestChangedFilesController from '../controllers/pr-changed-files-controller'; +import LoadingView from '../views/loading-view'; + +export default class PullRequestChangedFilesContainer extends React.Component { + static propTypes = { + pullRequestURL: PropTypes.string.isRequired, + } + + constructor(props) { + super(props); + this.state = {isLoading: true}; + const patchDiffURL = this.generatePatchDiffURL(this.props.pullRequestURL); + this.fetchData(patchDiffURL); + } + + async fetchData(URL) { + const response = await fetch(URL); + if (response.ok) { + const data = await response.text(); + this.setState({isLoading: false, data}); + } else { + // todo: make error messages more user friendly / readable + this.setState({isLoading: false, error: response.statusText}); + } + } + + // https://github.com/atom/github/pull/1804 becomes something like + // https://patch-diff.githubusercontent.com/raw/atom/github/pull/1829.diff + generatePatchDiffURL(pullRequestURL) { + const prURL = new URL(pullRequestURL); + const splitPath = prURL.pathname.split('/'); + const orgName = splitPath[1]; + const repoName = splitPath[2]; + const pullRequestId = splitPath[4]; + return `https://patch-diff.githubusercontent.com/raw/${orgName}/${repoName}/pull/${pullRequestId}.difff`; + } + + render() { + if (this.state.isLoading) { + return ; + } + + if (this.state.error) { + return (
{this.state.error}
); + } + + return ( + + ); + } +} diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js new file mode 100644 index 0000000000..5d7d1923cd --- /dev/null +++ b/lib/controllers/pr-changed-files-controller.js @@ -0,0 +1,11 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +import PullRequestChangedFilesView from '../views/pr-changed-files-view'; + +export default class PullRequestChangedFilesController extends React.Component { + + render() { + return (); + } +} diff --git a/lib/views/pr-changed-files-view.js b/lib/views/pr-changed-files-view.js new file mode 100644 index 0000000000..8422cf87b0 --- /dev/null +++ b/lib/views/pr-changed-files-view.js @@ -0,0 +1,9 @@ +import React from 'react'; +import PropTypes from 'prop-types'; + +export default class PullRequestChangedFilesView extends React.Component { + + render() { + return (
yo yo yo
); + } +} diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index aa8c36b9d8..588de8c02c 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -14,6 +14,7 @@ import PeriodicRefresher from '../periodic-refresher'; import {EnableableOperationPropType} from '../prop-types'; import {addEvent} from '../reporter-proxy'; import PrCommitsView from '../views/pr-commits-view'; +import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; class CheckoutState { constructor(name) { @@ -141,7 +142,7 @@ export class BarePullRequestDetailView extends React.Component { Files Changed - {/* 'Reviews' tab to be added in the future. */} + {/* 'Reviews' tab to be added in the future. */} {/* overview */} @@ -171,8 +172,11 @@ export class BarePullRequestDetailView extends React.Component { - {/* files changed */} + {/* files changed */} + ); From 686cf43293845d4546f79627bc15ae56603ece03 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Fri, 7 Dec 2018 16:40:53 -0800 Subject: [PATCH 003/102] make patch URL actually valid I was testing error cases and forgot to fix it, oops. --- lib/containers/pr-changed-files-container.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index a9af2de70f..3722d36ab4 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -35,7 +35,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { const orgName = splitPath[1]; const repoName = splitPath[2]; const pullRequestId = splitPath[4]; - return `https://patch-diff.githubusercontent.com/raw/${orgName}/${repoName}/pull/${pullRequestId}.difff`; + return `https://patch-diff.githubusercontent.com/raw/${orgName}/${repoName}/pull/${pullRequestId}.diff`; } render() { From 9f378a55258e85035df89b680b947b9653ca87a5 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Fri, 7 Dec 2018 18:17:28 -0800 Subject: [PATCH 004/102] drill them props --- lib/controllers/issueish-detail-controller.js | 9 +++++++++ lib/controllers/pr-changed-files-controller.js | 5 ++++- lib/controllers/root-controller.js | 4 ++++ lib/items/issueish-detail-item.js | 4 ++++ lib/views/pr-detail-view.js | 6 ++++++ 5 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/controllers/issueish-detail-controller.js b/lib/controllers/issueish-detail-controller.js index c7c28d3027..7cbd2eb5d9 100644 --- a/lib/controllers/issueish-detail-controller.js +++ b/lib/controllers/issueish-detail-controller.js @@ -39,6 +39,10 @@ export class BareIssueishDetailController extends React.Component { workspace: PropTypes.object.isRequired, workdirPath: PropTypes.string.isRequired, + commands: PropTypes.object.isRequired, + keymaps: PropTypes.object.isRequired, + tooltips: PropTypes.object.isRequired, + config: PropTypes.object.isRequired, } constructor(props) { @@ -113,6 +117,11 @@ export class BareIssueishDetailController extends React.Component { checkoutOp={this.checkoutOp} switchToIssueish={this.props.switchToIssueish} openCommit={this.openCommit} + workspace={this.props.workspace} + commands={this.props.commands} + keymaps={this.props.keymaps} + tooltips={this.props.tooltips} + config={this.props.config} /> ); } else { diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index 5d7d1923cd..a9c24622f5 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -6,6 +6,9 @@ import PullRequestChangedFilesView from '../views/pr-changed-files-view'; export default class PullRequestChangedFilesController extends React.Component { render() { - return (); + return ( + ); } } diff --git a/lib/controllers/root-controller.js b/lib/controllers/root-controller.js index 264f0d8333..ec67dbfa07 100644 --- a/lib/controllers/root-controller.js +++ b/lib/controllers/root-controller.js @@ -419,6 +419,10 @@ export default class RootController extends React.Component { loginModel={this.props.loginModel} workspace={this.props.workspace} + commands={this.props.commandRegistry} + keymaps={this.props.keymaps} + tooltips={this.props.tooltips} + config={this.props.config} /> )} diff --git a/lib/items/issueish-detail-item.js b/lib/items/issueish-detail-item.js index 88c0bd78d3..b569a7a1da 100644 --- a/lib/items/issueish-detail-item.js +++ b/lib/items/issueish-detail-item.js @@ -73,6 +73,10 @@ export default class IssueishDetailItem extends Component { onTitleChange={this.handleTitleChanged} switchToIssueish={this.switchToIssueish} + commands={this.props.commands} + keymaps={this.props.keymaps} + tooltips={this.props.tooltips} + config={this.props.config} /> ); } diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 588de8c02c..2abd031990 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -176,6 +176,12 @@ export class BarePullRequestDetailView extends React.Component { From 4d2098daff9090a650c9ede0e54340aa8d099a2d Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Fri, 7 Dec 2018 18:17:57 -0800 Subject: [PATCH 005/102] drill these props too --- lib/containers/issueish-detail-container.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/containers/issueish-detail-container.js b/lib/containers/issueish-detail-container.js index ff9e419b80..fbf6c5b074 100644 --- a/lib/containers/issueish-detail-container.js +++ b/lib/containers/issueish-detail-container.js @@ -172,7 +172,12 @@ export default class IssueishDetailContainer extends React.Component { onTitleChange={this.props.onTitleChange} switchToIssueish={this.props.switchToIssueish} workdirPath={repository.getWorkingDirectoryPath()} + workspace={this.props.workspace} + commands={this.props.commands} + keymaps={this.props.keymaps} + tooltips={this.props.tooltips} + config={this.props.config} /> ); } From e3fb672a6f73e7d8cba624a2a3ce753ff6d6c066 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Fri, 7 Dec 2018 18:22:13 -0800 Subject: [PATCH 006/102] [wip] actually build the file patch --- lib/containers/pr-changed-files-container.js | 3 ++- lib/controllers/pr-changed-files-controller.js | 13 ++++++++++++- lib/views/pr-changed-files-view.js | 11 ++++++++++- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 3722d36ab4..13c9ba4f1f 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -16,6 +16,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { this.fetchData(patchDiffURL); } + // todo: deal with when we need to refetch data async fetchData(URL) { const response = await fetch(URL); if (response.ok) { @@ -49,7 +50,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { return ( ); diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index a9c24622f5..a2eb8835fc 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -1,14 +1,25 @@ import React from 'react'; import PropTypes from 'prop-types'; +import {buildMultiFilePatch} from '../models/patch'; + import PullRequestChangedFilesView from '../views/pr-changed-files-view'; export default class PullRequestChangedFilesController extends React.Component { + buildPatch() { + // this no worky + // how do I build a filepatch from a string? + // also, we don't need to rebuild this on render, dawg. + return buildMultiFilePatch(this.props.data); + } + render() { return ( ); + /> + ); } } diff --git a/lib/views/pr-changed-files-view.js b/lib/views/pr-changed-files-view.js index 8422cf87b0..37f4ac0e8e 100644 --- a/lib/views/pr-changed-files-view.js +++ b/lib/views/pr-changed-files-view.js @@ -1,9 +1,18 @@ import React from 'react'; import PropTypes from 'prop-types'; +import MultiFilePatchController from '../controllers/multi-file-patch-controller'; + export default class PullRequestChangedFilesView extends React.Component { render() { - return (
yo yo yo
); + return ( +
+ +
); } } From 58f00d00ba0584b1922a4a8afdf6985e00ecb7ab Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 10 Dec 2018 17:02:34 -0800 Subject: [PATCH 007/102] hacky fix for height problem --- lib/views/pr-detail-view.js | 2 +- styles/issueish-detail-view.less | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 2abd031990..0aee176662 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -173,7 +173,7 @@ export class BarePullRequestDetailView extends React.Component {
{/* files changed */} - + Date: Mon, 10 Dec 2018 17:03:31 -0800 Subject: [PATCH 008/102] pass `itemType` to all the components that need it Co-Authored-By: Katrina Uychaco --- lib/containers/pr-changed-files-container.js | 1 + lib/views/file-patch-header-view.js | 5 +++-- lib/views/hunk-header-view.js | 5 +++-- lib/views/multi-file-patch-view.js | 9 ++++++--- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 13c9ba4f1f..8502d69854 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -52,6 +52,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { ); } diff --git a/lib/views/file-patch-header-view.js b/lib/views/file-patch-header-view.js index 51131689f0..6a429ecff3 100644 --- a/lib/views/file-patch-header-view.js +++ b/lib/views/file-patch-header-view.js @@ -8,6 +8,7 @@ import RefHolder from '../models/ref-holder'; import ChangedFileItem from '../items/changed-file-item'; import CommitPreviewItem from '../items/commit-preview-item'; import CommitDetailItem from '../items/commit-detail-item'; +import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; export default class FilePatchHeaderView extends React.Component { static propTypes = { @@ -25,7 +26,7 @@ export default class FilePatchHeaderView extends React.Component { openFile: PropTypes.func.isRequired, toggleFile: PropTypes.func.isRequired, - itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem]).isRequired, + itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem, PullRequestChangedFilesContainer]).isRequired, }; constructor(props) { @@ -73,7 +74,7 @@ export default class FilePatchHeaderView extends React.Component { } renderButtonGroup() { - if (this.props.itemType === CommitDetailItem) { + if (this.props.itemType === CommitDetailItem || this.props.itemType === PullRequestChangedFilesContainer) { return null; } else { return ( diff --git a/lib/views/hunk-header-view.js b/lib/views/hunk-header-view.js index a2ed357015..4d98a05613 100644 --- a/lib/views/hunk-header-view.js +++ b/lib/views/hunk-header-view.js @@ -10,6 +10,7 @@ import Keystroke from '../atom/keystroke'; import ChangedFileItem from '../items/changed-file-item'; import CommitPreviewItem from '../items/commit-preview-item'; import CommitDetailItem from '../items/commit-detail-item'; +import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; function theBuckStopsHere(event) { event.stopPropagation(); @@ -31,7 +32,7 @@ export default class HunkHeaderView extends React.Component { toggleSelection: PropTypes.func, discardSelection: PropTypes.func, mouseDown: PropTypes.func.isRequired, - itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem]).isRequired, + itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem, PullRequestChangedFilesContainer]).isRequired, }; constructor(props) { @@ -58,7 +59,7 @@ export default class HunkHeaderView extends React.Component { } renderButtons() { - if (this.props.itemType === CommitDetailItem) { + if (this.props.itemType === CommitDetailItem || this.props.itemType === PullRequestChangedFilesContainer) { return null; } else { return ( diff --git a/lib/views/multi-file-patch-view.js b/lib/views/multi-file-patch-view.js index 5d1407e496..41adc244d6 100644 --- a/lib/views/multi-file-patch-view.js +++ b/lib/views/multi-file-patch-view.js @@ -19,6 +19,8 @@ import RefHolder from '../models/ref-holder'; import ChangedFileItem from '../items/changed-file-item'; import CommitPreviewItem from '../items/commit-preview-item'; import CommitDetailItem from '../items/commit-detail-item'; +// todo: should we be using a PullRequestChangedFileItem just to make this consistent? +import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; import File from '../models/patch/file'; const executableText = { @@ -59,7 +61,7 @@ export default class MultiFilePatchView extends React.Component { undoLastDiscard: PropTypes.func, discardRows: PropTypes.func, refInitialFocus: RefHolderPropType, - itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem]).isRequired, + itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem, PullRequestChangedFilesContainer]).isRequired, } constructor(props) { @@ -169,7 +171,7 @@ export default class MultiFilePatchView extends React.Component { render() { const rootClass = cx( 'github-FilePatchView', - `github-FilePatchView--${this.props.stagingStatus}`, + {[`github-FilePatchView--${this.props.stagingStatus}`]: this.props.stagingStatus}, {'github-FilePatchView--blank': !this.props.multiFilePatch.anyPresent()}, {'github-FilePatchView--hunkMode': this.props.selectionMode === 'hunk'}, ); @@ -186,11 +188,12 @@ export default class MultiFilePatchView extends React.Component { } renderCommands() { - if (this.props.itemType === CommitDetailItem) { + if (this.props.itemType === CommitDetailItem || this.props.itemType === PullRequestChangedFilesContainer) { return ( + ); } From 2a2281655e0a7f205890bc7d7ec72463c0927370 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 10 Dec 2018 17:04:06 -0800 Subject: [PATCH 009/102] jeez, you want more props? so needy. Co-Authored-By: Katrina Uychaco --- .../pr-changed-files-controller.js | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index a2eb8835fc..b0687c29e5 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -2,22 +2,35 @@ import React from 'react'; import PropTypes from 'prop-types'; import {buildMultiFilePatch} from '../models/patch'; +import {parse as parseDiff} from 'what-the-diff'; import PullRequestChangedFilesView from '../views/pr-changed-files-view'; export default class PullRequestChangedFilesController extends React.Component { - buildPatch() { - // this no worky - // how do I build a filepatch from a string? + buildPatch(rawDiff) { // also, we don't need to rebuild this on render, dawg. - return buildMultiFilePatch(this.props.data); + // okay, probably I should extract this to a model somehow + // feels wrong to do it in the controller. + const diffs = parseDiff(rawDiff); + return buildMultiFilePatch(diffs); + } + + // todo: should this function go in the Container since we aren't + // using an `item` for this component tree? + destroy = () => { + /* istanbul ignore else */ + // if (!this.isDestroyed) { + // this.emitter.emit('did-destroy'); + // this.isDestroyed = true; + // } } render() { return ( ); From acd962670447aac8f598cac7dd75bf0f1cb746c8 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 10 Dec 2018 17:04:34 -0800 Subject: [PATCH 010/102] smell ya later unnecessary div Co-Authored-By: Katrina Uychaco --- lib/views/pr-changed-files-view.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/views/pr-changed-files-view.js b/lib/views/pr-changed-files-view.js index 37f4ac0e8e..1a59aadcf1 100644 --- a/lib/views/pr-changed-files-view.js +++ b/lib/views/pr-changed-files-view.js @@ -4,15 +4,15 @@ import PropTypes from 'prop-types'; import MultiFilePatchController from '../controllers/multi-file-patch-controller'; export default class PullRequestChangedFilesView extends React.Component { + constructor(props) { + super(props); + } render() { return ( -
- -
); + ); } } From 5041b71a659abfd2e02c8abc6eaaa6c797695275 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 10 Dec 2018 17:51:15 -0800 Subject: [PATCH 011/102] add some tests for `PullRequestChangedFilesoContainer` --- .../pr-changed-files-container.test.js | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 test/containers/pr-changed-files-container.test.js diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js new file mode 100644 index 0000000000..bf9e64839d --- /dev/null +++ b/test/containers/pr-changed-files-container.test.js @@ -0,0 +1,56 @@ +import React from 'react'; +import {shallow, mount} from 'enzyme'; + +import PullRequestChangedFilesContainer from '../../lib/containers/pr-changed-files-container'; + +describe('PullRequestChangedFilesContainer', function() { + + function buildApp(overrideProps = {}) { + return ( + + ); + } + it('passes extra props through to PullRequestChangedFilesController', function() { + const extraProp = Symbol('really really extra'); + + const wrapper = shallow(buildApp({extraProp})); + wrapper.instance().setState({isLoading: false}); + + const controller = wrapper.find('PullRequestChangedFilesController'); + + assert.strictEqual(controller.prop('extraProp'), extraProp); + }); + + it('passes itemType prop to PullRequestChangedFilesController', function() { + const wrapper = shallow(buildApp()); + wrapper.instance().setState({isLoading: false}); + + const controller = wrapper.find('PullRequestChangedFilesController'); + + assert.strictEqual(controller.prop('itemType'), PullRequestChangedFilesContainer); + }); + + it('renders a loading spinner if data has not yet been fetched', function() { + const wrapper = shallow(buildApp()); + assert.isTrue(wrapper.find('LoadingView').exists()); + }); + + it.only('builds the diff URL', function() { + const wrapper = shallow(buildApp()); + const pullRequestURL = 'https://github.com/atom/github/pull/1804'; + const diffURL = wrapper.instance().generatePatchDiffURL(pullRequestURL); + console.log(diffURL); + assert.strictEqual(diffURL, 'https://patch-diff.githubusercontent.com/raw/atom/github/pull/1804.diff'); + }); + + it('fetches data', function() { + }); + + it('renders an error if fetch returns a non-ok response', function() { + + }); + +}); From f344abb9e113fdf8bdc7b53451fc00e524ba89aa Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 10 Dec 2018 17:53:32 -0800 Subject: [PATCH 012/102] love too commit .only and console logging --- test/containers/pr-changed-files-container.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index bf9e64839d..aa401d2390 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -38,11 +38,10 @@ describe('PullRequestChangedFilesContainer', function() { assert.isTrue(wrapper.find('LoadingView').exists()); }); - it.only('builds the diff URL', function() { + it('builds the diff URL', function() { const wrapper = shallow(buildApp()); const pullRequestURL = 'https://github.com/atom/github/pull/1804'; const diffURL = wrapper.instance().generatePatchDiffURL(pullRequestURL); - console.log(diffURL); assert.strictEqual(diffURL, 'https://patch-diff.githubusercontent.com/raw/atom/github/pull/1804.diff'); }); From edd2b0789e9c85822c56efc6c45814e0df057cee Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 10 Dec 2018 18:28:08 -0800 Subject: [PATCH 013/102] moar tests! moar! --- .../pr-changed-files-container.test.js | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index aa401d2390..7f83bc0d82 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -1,5 +1,5 @@ import React from 'react'; -import {shallow, mount} from 'enzyme'; +import {shallow} from 'enzyme'; import PullRequestChangedFilesContainer from '../../lib/containers/pr-changed-files-container'; @@ -20,7 +20,6 @@ describe('PullRequestChangedFilesContainer', function() { wrapper.instance().setState({isLoading: false}); const controller = wrapper.find('PullRequestChangedFilesController'); - assert.strictEqual(controller.prop('extraProp'), extraProp); }); @@ -33,6 +32,18 @@ describe('PullRequestChangedFilesContainer', function() { assert.strictEqual(controller.prop('itemType'), PullRequestChangedFilesContainer); }); + it('passes data prop through to PullRequestChangedFilesContainer', function() { + const fakeData = 'some really swell diff'; + const wrapper = shallow(buildApp()); + wrapper.instance().setState({isLoading: false}); + wrapper.instance().setState({data: fakeData}); + + const controller = wrapper.find('PullRequestChangedFilesController'); + + assert.strictEqual(controller.prop('data'), fakeData); + }); + + it('renders a loading spinner if data has not yet been fetched', function() { const wrapper = shallow(buildApp()); assert.isTrue(wrapper.find('LoadingView').exists()); @@ -45,7 +56,22 @@ describe('PullRequestChangedFilesContainer', function() { assert.strictEqual(diffURL, 'https://patch-diff.githubusercontent.com/raw/atom/github/pull/1804.diff'); }); - it('fetches data', function() { + it('fetches data and sets it in state', async function() { + const stubbedFetch = sinon.stub(window, 'fetch'); + + window.fetch.returns(Promise.resolve(mockApiResponse())); + function mockApiResponse(body = 'oh em gee') { + return new window.Response(JSON.stringify(body), { + status: 200, + headers: {'Content-type': 'text/plain'}, + }); + } + const wrapper = shallow(buildApp()); + await assert.async.isFalse(wrapper.instance().state.isLoading); + assert.strictEqual(stubbedFetch.callCount, 1); + + assert.deepEqual(stubbedFetch.lastCall.args, ['https://patch-diff.githubusercontent.com/raw/atom/github/pull/1804.diff']); + assert.strictEqual(wrapper.instance().state.data, '"oh em gee"'); }); it('renders an error if fetch returns a non-ok response', function() { From 8733b9b49f03cb8fab46914b606d990cc1206a50 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 12:21:27 -0500 Subject: [PATCH 014/102] Document and reorganize PropTypes a bit --- lib/containers/issueish-detail-container.js | 12 +++++++++-- lib/controllers/issueish-detail-controller.js | 20 +++++++++++++------ .../pr-changed-files-controller.js | 3 +++ lib/items/issueish-detail-item.js | 10 +++++++++- lib/views/pr-detail-view.js | 14 +++++++++++++ 5 files changed, 50 insertions(+), 9 deletions(-) diff --git a/lib/containers/issueish-detail-container.js b/lib/containers/issueish-detail-container.js index fbf6c5b074..c9f012aae1 100644 --- a/lib/containers/issueish-detail-container.js +++ b/lib/containers/issueish-detail-container.js @@ -16,18 +16,26 @@ import {autobind} from '../helpers'; export default class IssueishDetailContainer extends React.Component { static propTypes = { + // Issueish selection criteria host: PropTypes.string, owner: PropTypes.string.isRequired, repo: PropTypes.string.isRequired, issueishNumber: PropTypes.number.isRequired, + // Package models repository: PropTypes.object.isRequired, loginModel: GithubLoginModelPropType.isRequired, + // Atom environment + workspace: PropTypes.object.isRequired, + commands: PropTypes.object.isRequired, + keymaps: PropTypes.object.isRequired, + tooltips: PropTypes.object.isRequired, + config: PropTypes.object.isRequired, + + // Action methods switchToIssueish: PropTypes.func.isRequired, onTitleChange: PropTypes.func.isRequired, - - workspace: PropTypes.object.isRequired, } constructor(props) { diff --git a/lib/controllers/issueish-detail-controller.js b/lib/controllers/issueish-detail-controller.js index 7cbd2eb5d9..6e32ef273b 100644 --- a/lib/controllers/issueish-detail-controller.js +++ b/lib/controllers/issueish-detail-controller.js @@ -12,6 +12,7 @@ import {incrementCounter, addEvent} from '../reporter-proxy'; export class BareIssueishDetailController extends React.Component { static propTypes = { + // Relay response repository: PropTypes.shape({ name: PropTypes.string.isRequired, owner: PropTypes.shape({ @@ -22,6 +23,7 @@ export class BareIssueishDetailController extends React.Component { }), issueishNumber: PropTypes.number.isRequired, + // Local Repository model properties branches: BranchSetPropType.isRequired, remotes: RemoteSetPropType.isRequired, isMerging: PropTypes.bool.isRequired, @@ -30,19 +32,25 @@ export class BareIssueishDetailController extends React.Component { isLoading: PropTypes.bool.isRequired, isPresent: PropTypes.bool.isRequired, - fetch: PropTypes.func.isRequired, - checkout: PropTypes.func.isRequired, - pull: PropTypes.func.isRequired, - addRemote: PropTypes.func.isRequired, - onTitleChange: PropTypes.func.isRequired, - switchToIssueish: PropTypes.func.isRequired, + // Connection information + host: PropTypes.string.isRequired, + token: PropTypes.string.isRequired, + // Atom environment workspace: PropTypes.object.isRequired, workdirPath: PropTypes.string.isRequired, commands: PropTypes.object.isRequired, keymaps: PropTypes.object.isRequired, tooltips: PropTypes.object.isRequired, config: PropTypes.object.isRequired, + + // Action methods + fetch: PropTypes.func.isRequired, + checkout: PropTypes.func.isRequired, + pull: PropTypes.func.isRequired, + addRemote: PropTypes.func.isRequired, + onTitleChange: PropTypes.func.isRequired, + switchToIssueish: PropTypes.func.isRequired, } constructor(props) { diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index b0687c29e5..f1f7375818 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -7,6 +7,9 @@ import {parse as parseDiff} from 'what-the-diff'; import PullRequestChangedFilesView from '../views/pr-changed-files-view'; export default class PullRequestChangedFilesController extends React.Component { + static propTypes = { + diff: PropTypes.string.isRequired, + } buildPatch(rawDiff) { // also, we don't need to rebuild this on render, dawg. diff --git a/lib/items/issueish-detail-item.js b/lib/items/issueish-detail-item.js index b569a7a1da..e7d448b25c 100644 --- a/lib/items/issueish-detail-item.js +++ b/lib/items/issueish-detail-item.js @@ -10,16 +10,24 @@ import {addEvent} from '../reporter-proxy'; export default class IssueishDetailItem extends Component { static propTypes = { + // Issueish selection criteria + // Parsed from item URI host: PropTypes.string.isRequired, owner: PropTypes.string.isRequired, repo: PropTypes.string.isRequired, issueishNumber: PropTypes.number.isRequired, - workingDirectory: PropTypes.string.isRequired, + + // Package models workdirContextPool: WorkdirContextPoolPropType.isRequired, loginModel: GithubLoginModelPropType.isRequired, + // Atom environment workspace: PropTypes.object.isRequired, + commands: PropTypes.object.isRequired, + keymaps: PropTypes.object.isRequired, + tooltips: PropTypes.object.isRequired, + config: PropTypes.object.isRequired, } static uriPattern = 'atom-github://issueish/{host}/{owner}/{repo}/{issueishNumber}?workdir={workingDirectory}' diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 0aee176662..f55a06bc65 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -35,6 +35,7 @@ export const checkoutStates = { export class BarePullRequestDetailView extends React.Component { static propTypes = { + // Relay response relay: PropTypes.shape({ refetch: PropTypes.func.isRequired, }), @@ -76,6 +77,19 @@ export class BarePullRequestDetailView extends React.Component { }), ).isRequired, }).isRequired, + + // Connection information + host: PropTypes.string.isRequired, + token: PropTypes.string.isRequired, + + // Atom environment + workspace: PropTypes.object.isRequired, + commands: PropTypes.object.isRequired, + keymaps: PropTypes.object.isRequired, + tooltips: PropTypes.object.isRequired, + config: PropTypes.object.isRequired, + + // Action functions openCommit: PropTypes.func.isRequired, } From ddea33e209d7982db750aedc90cf525ead0d7c47 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 12:23:53 -0500 Subject: [PATCH 015/102] Pass the host and token through IssueishDetail components --- lib/containers/issueish-detail-container.js | 7 +++++-- lib/controllers/issueish-detail-controller.js | 7 ++++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/containers/issueish-detail-container.js b/lib/containers/issueish-detail-container.js index c9f012aae1..f7bd609d77 100644 --- a/lib/containers/issueish-detail-container.js +++ b/lib/containers/issueish-detail-container.js @@ -144,13 +144,13 @@ export default class IssueishDetailContainer extends React.Component { environment={environment} query={query} variables={variables} - render={queryResult => this.renderWithResult(queryResult, repoData)} + render={queryResult => this.renderWithResult(queryResult, repoData, token)} /> ); } - renderWithResult({error, props, retry}, repoData) { + renderWithResult({error, props, retry}, repoData, token) { if (error) { return ( ); } else { From 314439e9ed830081854ede01844b384235a39622 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 12:24:38 -0500 Subject: [PATCH 016/102] Shuffle a bunch of stuff in PrChangedFilesContainer because I'm me --- lib/containers/pr-changed-files-container.js | 54 ++++++---- .../pr-changed-files-container.test.js | 102 ++++++++++-------- 2 files changed, 88 insertions(+), 68 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 8502d69854..82044b2709 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -6,39 +6,49 @@ import LoadingView from '../views/loading-view'; export default class PullRequestChangedFilesContainer extends React.Component { static propTypes = { - pullRequestURL: PropTypes.string.isRequired, + // Pull request properties + owner: PropTypes.string.isRequired, + repo: PropTypes.string.isRequired, + number: PropTypes.number.isRequired, + + // Connection properties + token: PropTypes.string.isRequired, + host: PropTypes.string.isRequired, } constructor(props) { super(props); this.state = {isLoading: true}; - const patchDiffURL = this.generatePatchDiffURL(this.props.pullRequestURL); - this.fetchData(patchDiffURL); + this.fetchDiff(); + } + + // Generate a v3 GitHub API REST URL for the pull request resource. + // Example: https://api.github.com/repos/atom/github/pulls/1829 + getDiffURL() { + // TODO centralize endpoint translation logic between here and lib/relay-network-layer-manager.js. + // Maybe move it to the Remote model instead? + const endpoint = this.props.host === 'github.com' ? 'https://api.github.com' : this.props.host; + return `${endpoint}/repos/${this.props.owner}/${this.props.repo}/pulls/${this.props.number}`; } - // todo: deal with when we need to refetch data - async fetchData(URL) { - const response = await fetch(URL); + // TODO: deal with when we need to refetch data + async fetchDiff() { + const url = this.getDiffURL(); + const response = await fetch(url, { + headers: { + Accept: 'application/vnd.github.v3.diff', + Authorization: `bearer ${this.props.token}`, + }, + }); if (response.ok) { - const data = await response.text(); - this.setState({isLoading: false, data}); + const diff = await response.text(); + await new Promise(resolve => this.setState({isLoading: false, diff}, resolve)); } else { - // todo: make error messages more user friendly / readable - this.setState({isLoading: false, error: response.statusText}); + // TODO: make error messages more user friendly / readable + await new Promise(resolve => this.setState({isLoading: false, error: response.statusText}, resolve)); } } - // https://github.com/atom/github/pull/1804 becomes something like - // https://patch-diff.githubusercontent.com/raw/atom/github/pull/1829.diff - generatePatchDiffURL(pullRequestURL) { - const prURL = new URL(pullRequestURL); - const splitPath = prURL.pathname.split('/'); - const orgName = splitPath[1]; - const repoName = splitPath[2]; - const pullRequestId = splitPath[4]; - return `https://patch-diff.githubusercontent.com/raw/${orgName}/${repoName}/pull/${pullRequestId}.diff`; - } - render() { if (this.state.isLoading) { return ; @@ -50,7 +60,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { return ( diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index 7f83bc0d82..603e1d8540 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -4,78 +4,88 @@ import {shallow} from 'enzyme'; import PullRequestChangedFilesContainer from '../../lib/containers/pr-changed-files-container'; describe('PullRequestChangedFilesContainer', function() { + let diffResponse; function buildApp(overrideProps = {}) { return ( ); } - it('passes extra props through to PullRequestChangedFilesController', function() { - const extraProp = Symbol('really really extra'); - const wrapper = shallow(buildApp({extraProp})); - wrapper.instance().setState({isLoading: false}); + function setDiffResponse(body) { + diffResponse = new window.Response(JSON.stringify(body), { + status: 200, + headers: {'Content-type': 'text/plain'}, + }); + } - const controller = wrapper.find('PullRequestChangedFilesController'); - assert.strictEqual(controller.prop('extraProp'), extraProp); + beforeEach(function() { + setDiffResponse('default'); + sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); }); - it('passes itemType prop to PullRequestChangedFilesController', function() { + it('renders a loading spinner if data has not yet been fetched', function() { const wrapper = shallow(buildApp()); - wrapper.instance().setState({isLoading: false}); - - const controller = wrapper.find('PullRequestChangedFilesController'); - - assert.strictEqual(controller.prop('itemType'), PullRequestChangedFilesContainer); + assert.isTrue(wrapper.find('LoadingView').exists()); }); - it('passes data prop through to PullRequestChangedFilesContainer', function() { - const fakeData = 'some really swell diff'; - const wrapper = shallow(buildApp()); - wrapper.instance().setState({isLoading: false}); - wrapper.instance().setState({data: fakeData}); + it('passes extra props through to PullRequestChangedFilesController', async function() { + const extraProp = Symbol('really really extra'); - const controller = wrapper.find('PullRequestChangedFilesController'); + const wrapper = shallow(buildApp({extraProp})); + await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); - assert.strictEqual(controller.prop('data'), fakeData); + const controller = wrapper.find('PullRequestChangedFilesController'); + assert.strictEqual(controller.prop('extraProp'), extraProp); }); - - it('renders a loading spinner if data has not yet been fetched', function() { + it('passes itemType prop to PullRequestChangedFilesController', async function() { const wrapper = shallow(buildApp()); - assert.isTrue(wrapper.find('LoadingView').exists()); - }); + await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); - it('builds the diff URL', function() { - const wrapper = shallow(buildApp()); - const pullRequestURL = 'https://github.com/atom/github/pull/1804'; - const diffURL = wrapper.instance().generatePatchDiffURL(pullRequestURL); - assert.strictEqual(diffURL, 'https://patch-diff.githubusercontent.com/raw/atom/github/pull/1804.diff'); + const controller = wrapper.find('PullRequestChangedFilesController'); + assert.strictEqual(controller.prop('itemType'), PullRequestChangedFilesContainer); }); - it('fetches data and sets it in state', async function() { - const stubbedFetch = sinon.stub(window, 'fetch'); - - window.fetch.returns(Promise.resolve(mockApiResponse())); - function mockApiResponse(body = 'oh em gee') { - return new window.Response(JSON.stringify(body), { - status: 200, - headers: {'Content-type': 'text/plain'}, - }); - } - const wrapper = shallow(buildApp()); - await assert.async.isFalse(wrapper.instance().state.isLoading); - assert.strictEqual(stubbedFetch.callCount, 1); - - assert.deepEqual(stubbedFetch.lastCall.args, ['https://patch-diff.githubusercontent.com/raw/atom/github/pull/1804.diff']); - assert.strictEqual(wrapper.instance().state.data, '"oh em gee"'); + it('builds the diff URL', function() { + const wrapper = shallow(buildApp({ + owner: 'smashwilson', + repo: 'pushbot', + number: 12, + host: 'github.com', + })); + + const diffURL = wrapper.instance().getDiffURL(); + assert.strictEqual(diffURL, 'https://api.github.com/repos/smashwilson/pushbot/pulls/12'); }); - it('renders an error if fetch returns a non-ok response', function() { + it('passes loaded diff data through to the controller', async function() { + setDiffResponse('some really swell diff'); + const wrapper = shallow(buildApp({ + token: '4321', + })); + await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); + const controller = wrapper.find('PullRequestChangedFilesController'); + assert.strictEqual(controller.prop('diff'), '"some really swell diff"'); + + assert.deepEqual(window.fetch.lastCall.args, [ + 'https://api.github.com/repos/atom/github/pulls/1804', + { + headers: { + Accept: 'application/vnd.github.v3.diff', + Authorization: 'bearer 4321', + }, + }, + ]); }); + it('renders an error if fetch returns a non-ok response'); }); From 4401f8bef62ec2b099ae80003f88e694dfac89c6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 12:24:53 -0500 Subject: [PATCH 017/102] I'm calling that prop "diff" now --- lib/controllers/pr-changed-files-controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index f1f7375818..900240adcb 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -32,7 +32,7 @@ export default class PullRequestChangedFilesController extends React.Component { render() { return ( From 081264b94116ca02610de171114d436a0a620cf6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 12:25:23 -0500 Subject: [PATCH 018/102] Pass pull request attributes directly rather than parsing them from the URL --- lib/views/pr-detail-view.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index f55a06bc65..6f85f3f443 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -189,8 +189,12 @@ export class BarePullRequestDetailView extends React.Component { {/* files changed */} Date: Wed, 12 Dec 2018 12:25:32 -0500 Subject: [PATCH 019/102] Capital TODO to trigger TODO bot --- lib/controllers/pr-changed-files-controller.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index 900240adcb..ea9a1956cf 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -19,8 +19,7 @@ export default class PullRequestChangedFilesController extends React.Component { return buildMultiFilePatch(diffs); } - // todo: should this function go in the Container since we aren't - // using an `item` for this component tree? + // TODO: should this function go in the Container since we aren't using an `item` for this component tree? destroy = () => { /* istanbul ignore else */ // if (!this.isDestroyed) { From 61920e05342f883ec93fd07234d2fa16cc542a06 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 13:44:59 -0500 Subject: [PATCH 020/102] Drill that Repository down to the MultiFilePatch components --- lib/containers/issueish-detail-container.js | 1 + lib/controllers/issueish-detail-controller.js | 2 ++ lib/views/pr-changed-files-view.js | 6 ++++-- lib/views/pr-detail-view.js | 6 ++++++ 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/containers/issueish-detail-container.js b/lib/containers/issueish-detail-container.js index f7bd609d77..70173062d3 100644 --- a/lib/containers/issueish-detail-container.js +++ b/lib/containers/issueish-detail-container.js @@ -172,6 +172,7 @@ export default class IssueishDetailContainer extends React.Component { ); diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 6f85f3f443..fe042ea4d5 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -78,6 +78,9 @@ export class BarePullRequestDetailView extends React.Component { ).isRequired, }).isRequired, + // Local model objects + localRepository: PropTypes.object.isRequired, + // Connection information host: PropTypes.string.isRequired, token: PropTypes.string.isRequired, @@ -189,9 +192,12 @@ export class BarePullRequestDetailView extends React.Component { {/* files changed */} Date: Wed, 12 Dec 2018 15:42:41 -0500 Subject: [PATCH 021/102] Overkill custom PropType validator for itemType For a little context, I couldn't use `PropTypes.oneOf()` directly because multiple Item classes themselves use custom PropTypes defined within this file. That led to a circular import which caused the PropType to check values against a set of `undefined` values. --- lib/prop-types.js | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/lib/prop-types.js b/lib/prop-types.js index f00c709f8f..4f55860eb8 100644 --- a/lib/prop-types.js +++ b/lib/prop-types.js @@ -155,3 +155,40 @@ export const UserStorePropType = PropTypes.shape({ getUsers: PropTypes.func.isRequired, onDidUpdate: PropTypes.func.isRequired, }); + +// Require item classes lazily to prevent circular imports +let lazyItemConstructors = null; +function createItemTypePropType(required) { + return function(props, propName, componentName) { + if (lazyItemConstructors === null) { + lazyItemConstructors = new Set(); + for (const itemPath of [ + './items/changed-file-item', + './items/commit-preview-item', + './items/commit-detail-item', + './items/issueish-detail-item', + ]) { + lazyItemConstructors.add(require(itemPath).default); + } + } + + if (props[propName] === undefined || props[propName] === null) { + if (required) { + return new Error(`Missing required prop ${propName} on component ${componentName}.`); + } else { + return undefined; + } + } + + if (!lazyItemConstructors.has(props[propName])) { + const choices = Array.from(lazyItemConstructors, each => each.name).join(', '); + return new Error( + `Invalid prop "${propName}" supplied to ${componentName}. Must be one of ${choices}.`); + } + + return undefined; + }; +} + +export const ItemTypePropType = createItemTypePropType(false); +ItemTypePropType.isRequired = createItemTypePropType(true); From 8d5a8450843f018e0e2327f2dd06411ce4c061c4 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 15:44:06 -0500 Subject: [PATCH 022/102] Use itemType within the IssueishDetail item --- lib/containers/issueish-detail-container.js | 7 ++++++- lib/containers/pr-changed-files-container.js | 14 ++++++-------- lib/controllers/issueish-detail-controller.js | 7 ++++++- lib/items/issueish-detail-item.js | 2 ++ lib/views/file-patch-header-view.js | 6 +++--- lib/views/file-patch-meta-view.js | 6 +++--- lib/views/hunk-header-view.js | 10 ++++------ lib/views/multi-file-patch-view.js | 10 ++++------ lib/views/pr-detail-view.js | 7 ++++++- 9 files changed, 40 insertions(+), 29 deletions(-) diff --git a/lib/containers/issueish-detail-container.js b/lib/containers/issueish-detail-container.js index 70173062d3..5f8e12a147 100644 --- a/lib/containers/issueish-detail-container.js +++ b/lib/containers/issueish-detail-container.js @@ -4,7 +4,7 @@ import yubikiri from 'yubikiri'; import {QueryRenderer, graphql} from 'react-relay'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; -import {GithubLoginModelPropType} from '../prop-types'; +import {GithubLoginModelPropType, ItemTypePropType} from '../prop-types'; import {UNAUTHENTICATED, INSUFFICIENT} from '../shared/keytar-strategy'; import GithubLoginView from '../views/github-login-view'; import LoadingView from '../views/loading-view'; @@ -36,6 +36,9 @@ export default class IssueishDetailContainer extends React.Component { // Action methods switchToIssueish: PropTypes.func.isRequired, onTitleChange: PropTypes.func.isRequired, + + // Item context + itemType: ItemTypePropType.isRequired, } constructor(props) { @@ -190,6 +193,8 @@ export default class IssueishDetailContainer extends React.Component { keymaps={this.props.keymaps} tooltips={this.props.tooltips} config={this.props.config} + + itemType={this.props.itemType} /> ); } diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 82044b2709..896620ca45 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -1,6 +1,7 @@ import React from 'react'; import PropTypes from 'prop-types'; +import {ItemTypePropType} from '../prop-types'; import PullRequestChangedFilesController from '../controllers/pr-changed-files-controller'; import LoadingView from '../views/loading-view'; @@ -14,6 +15,9 @@ export default class PullRequestChangedFilesContainer extends React.Component { // Connection properties token: PropTypes.string.isRequired, host: PropTypes.string.isRequired, + + // Item context + itemType: ItemTypePropType.isRequired, } constructor(props) { @@ -55,15 +59,9 @@ export default class PullRequestChangedFilesContainer extends React.Component { } if (this.state.error) { - return (
{this.state.error}
); + return
{this.state.error}
; } - return ( - - ); + return ; } } diff --git a/lib/controllers/issueish-detail-controller.js b/lib/controllers/issueish-detail-controller.js index a936769ae4..0975c855ed 100644 --- a/lib/controllers/issueish-detail-controller.js +++ b/lib/controllers/issueish-detail-controller.js @@ -2,7 +2,7 @@ import React from 'react'; import {graphql, createFragmentContainer} from 'react-relay'; import PropTypes from 'prop-types'; -import {BranchSetPropType, RemoteSetPropType} from '../prop-types'; +import {BranchSetPropType, RemoteSetPropType, ItemTypePropType} from '../prop-types'; import {GitError} from '../git-shell-out-strategy'; import EnableableOperation from '../models/enableable-operation'; import PullRequestDetailView, {checkoutStates} from '../views/pr-detail-view'; @@ -52,6 +52,9 @@ export class BareIssueishDetailController extends React.Component { addRemote: PropTypes.func.isRequired, onTitleChange: PropTypes.func.isRequired, switchToIssueish: PropTypes.func.isRequired, + + // Item context + itemType: ItemTypePropType.isRequired, } constructor(props) { @@ -137,6 +140,8 @@ export class BareIssueishDetailController extends React.Component { config={this.props.config} openCommit={this.openCommit} + + itemType={this.props.itemType} /> ); } else { diff --git a/lib/items/issueish-detail-item.js b/lib/items/issueish-detail-item.js index e7d448b25c..283c6535f9 100644 --- a/lib/items/issueish-detail-item.js +++ b/lib/items/issueish-detail-item.js @@ -85,6 +85,8 @@ export default class IssueishDetailItem extends Component { keymaps={this.props.keymaps} tooltips={this.props.tooltips} config={this.props.config} + + itemType={this.constructor} /> ); } diff --git a/lib/views/file-patch-header-view.js b/lib/views/file-patch-header-view.js index 6a429ecff3..6d0b6e3e04 100644 --- a/lib/views/file-patch-header-view.js +++ b/lib/views/file-patch-header-view.js @@ -5,10 +5,10 @@ import PropTypes from 'prop-types'; import cx from 'classnames'; import RefHolder from '../models/ref-holder'; +import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; import ChangedFileItem from '../items/changed-file-item'; -import CommitPreviewItem from '../items/commit-preview-item'; import CommitDetailItem from '../items/commit-detail-item'; -import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; +import {ItemTypePropType} from '../prop-types'; export default class FilePatchHeaderView extends React.Component { static propTypes = { @@ -26,7 +26,7 @@ export default class FilePatchHeaderView extends React.Component { openFile: PropTypes.func.isRequired, toggleFile: PropTypes.func.isRequired, - itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem, PullRequestChangedFilesContainer]).isRequired, + itemType: ItemTypePropType.isRequired, }; constructor(props) { diff --git a/lib/views/file-patch-meta-view.js b/lib/views/file-patch-meta-view.js index d39dbfcb60..10dd2e723a 100644 --- a/lib/views/file-patch-meta-view.js +++ b/lib/views/file-patch-meta-view.js @@ -1,9 +1,9 @@ import React from 'react'; import PropTypes from 'prop-types'; import cx from 'classnames'; + import CommitDetailItem from '../items/commit-detail-item'; -import ChangedFileItem from '../items/changed-file-item'; -import CommitPreviewItem from '../items/commit-preview-item'; +import {ItemTypePropType} from '../prop-types'; export default class FilePatchMetaView extends React.Component { static propTypes = { @@ -14,7 +14,7 @@ export default class FilePatchMetaView extends React.Component { action: PropTypes.func.isRequired, children: PropTypes.element.isRequired, - itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem]).isRequired, + itemType: ItemTypePropType.isRequired, }; renderMetaControls() { diff --git a/lib/views/hunk-header-view.js b/lib/views/hunk-header-view.js index 4d98a05613..89243a27c9 100644 --- a/lib/views/hunk-header-view.js +++ b/lib/views/hunk-header-view.js @@ -3,14 +3,12 @@ import PropTypes from 'prop-types'; import cx from 'classnames'; import {autobind} from '../helpers'; -import {RefHolderPropType} from '../prop-types'; +import {RefHolderPropType, ItemTypePropType} from '../prop-types'; import RefHolder from '../models/ref-holder'; import Tooltip from '../atom/tooltip'; import Keystroke from '../atom/keystroke'; -import ChangedFileItem from '../items/changed-file-item'; -import CommitPreviewItem from '../items/commit-preview-item'; import CommitDetailItem from '../items/commit-detail-item'; -import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; +import IssueishDetailItem from '../items/issueish-detail-item'; function theBuckStopsHere(event) { event.stopPropagation(); @@ -32,7 +30,7 @@ export default class HunkHeaderView extends React.Component { toggleSelection: PropTypes.func, discardSelection: PropTypes.func, mouseDown: PropTypes.func.isRequired, - itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem, PullRequestChangedFilesContainer]).isRequired, + itemType: ItemTypePropType.isRequired, }; constructor(props) { @@ -59,7 +57,7 @@ export default class HunkHeaderView extends React.Component { } renderButtons() { - if (this.props.itemType === CommitDetailItem || this.props.itemType === PullRequestChangedFilesContainer) { + if (this.props.itemType === CommitDetailItem || this.props.itemType === IssueishDetailItem) { return null; } else { return ( diff --git a/lib/views/multi-file-patch-view.js b/lib/views/multi-file-patch-view.js index 41adc244d6..b8d46976f8 100644 --- a/lib/views/multi-file-patch-view.js +++ b/lib/views/multi-file-patch-view.js @@ -5,7 +5,7 @@ import {Range} from 'atom'; import {CompositeDisposable} from 'event-kit'; import {autobind} from '../helpers'; -import {RefHolderPropType, MultiFilePatchPropType} from '../prop-types'; +import {RefHolderPropType, MultiFilePatchPropType, ItemTypePropType} from '../prop-types'; import AtomTextEditor from '../atom/atom-text-editor'; import Marker from '../atom/marker'; import MarkerLayer from '../atom/marker-layer'; @@ -17,10 +17,8 @@ import FilePatchMetaView from './file-patch-meta-view'; import HunkHeaderView from './hunk-header-view'; import RefHolder from '../models/ref-holder'; import ChangedFileItem from '../items/changed-file-item'; -import CommitPreviewItem from '../items/commit-preview-item'; import CommitDetailItem from '../items/commit-detail-item'; -// todo: should we be using a PullRequestChangedFileItem just to make this consistent? -import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; +import IssueishDetailItem from '../items/issueish-detail-item'; import File from '../models/patch/file'; const executableText = { @@ -61,7 +59,7 @@ export default class MultiFilePatchView extends React.Component { undoLastDiscard: PropTypes.func, discardRows: PropTypes.func, refInitialFocus: RefHolderPropType, - itemType: PropTypes.oneOf([ChangedFileItem, CommitPreviewItem, CommitDetailItem, PullRequestChangedFilesContainer]).isRequired, + itemType: ItemTypePropType.isRequired, } constructor(props) { @@ -188,7 +186,7 @@ export default class MultiFilePatchView extends React.Component { } renderCommands() { - if (this.props.itemType === CommitDetailItem || this.props.itemType === PullRequestChangedFilesContainer) { + if (this.props.itemType === CommitDetailItem || this.props.itemType === IssueishDetailItem) { return ( diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index fe042ea4d5..637e5aaaf1 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -11,7 +11,7 @@ import IssueishBadge from '../views/issueish-badge'; import GithubDotcomMarkdown from '../views/github-dotcom-markdown'; import EmojiReactionsView from '../views/emoji-reactions-view'; import PeriodicRefresher from '../periodic-refresher'; -import {EnableableOperationPropType} from '../prop-types'; +import {EnableableOperationPropType, ItemTypePropType} from '../prop-types'; import {addEvent} from '../reporter-proxy'; import PrCommitsView from '../views/pr-commits-view'; import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; @@ -94,6 +94,9 @@ export class BarePullRequestDetailView extends React.Component { // Action functions openCommit: PropTypes.func.isRequired, + + // Item context + itemType: ItemTypePropType.isRequired, } state = { @@ -206,6 +209,8 @@ export class BarePullRequestDetailView extends React.Component { keymaps={this.props.keymaps} tooltips={this.props.tooltips} config={this.props.config} + + itemType={this.props.itemType} />
From 99daac35ed6df9305255e0b7753c058e48126ce1 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 12:58:16 -0800 Subject: [PATCH 023/102] move diff parsing into `PrChangedFilesContainer` --- lib/containers/pr-changed-files-container.js | 30 +++++++++++++++++-- .../pr-changed-files-controller.js | 15 ++-------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 896620ca45..6b63b3244a 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -1,9 +1,11 @@ import React from 'react'; import PropTypes from 'prop-types'; +import {parse as parseDiff} from 'what-the-diff'; import {ItemTypePropType} from '../prop-types'; import PullRequestChangedFilesController from '../controllers/pr-changed-files-controller'; import LoadingView from '../views/loading-view'; +import {buildMultiFilePatch} from '../models/patch'; export default class PullRequestChangedFilesContainer extends React.Component { static propTypes = { @@ -35,6 +37,11 @@ export default class PullRequestChangedFilesContainer extends React.Component { return `${endpoint}/repos/${this.props.owner}/${this.props.repo}/pulls/${this.props.number}`; } + buildPatch(rawDiff) { + const diffs = parseDiff(rawDiff); + return buildMultiFilePatch(diffs); + } + // TODO: deal with when we need to refetch data async fetchDiff() { const url = this.getDiffURL(); @@ -45,8 +52,9 @@ export default class PullRequestChangedFilesContainer extends React.Component { }, }); if (response.ok) { - const diff = await response.text(); - await new Promise(resolve => this.setState({isLoading: false, diff}, resolve)); + const rawDiff = await response.text(); + const patch = this.buildPatch(rawDiff); + await new Promise(resolve => this.setState({isLoading: false, patch}, resolve)); } else { // TODO: make error messages more user friendly / readable await new Promise(resolve => this.setState({isLoading: false, error: response.statusText}, resolve)); @@ -62,6 +70,24 @@ export default class PullRequestChangedFilesContainer extends React.Component { return
{this.state.error}
; } +<<<<<<< Updated upstream return ; +||||||| merged common ancestors + return ( + + ); +======= + return ( + + ); +>>>>>>> Stashed changes } } diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index ea9a1956cf..e718c47942 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -1,22 +1,11 @@ import React from 'react'; import PropTypes from 'prop-types'; -import {buildMultiFilePatch} from '../models/patch'; -import {parse as parseDiff} from 'what-the-diff'; - import PullRequestChangedFilesView from '../views/pr-changed-files-view'; export default class PullRequestChangedFilesController extends React.Component { static propTypes = { - diff: PropTypes.string.isRequired, - } - - buildPatch(rawDiff) { - // also, we don't need to rebuild this on render, dawg. - // okay, probably I should extract this to a model somehow - // feels wrong to do it in the controller. - const diffs = parseDiff(rawDiff); - return buildMultiFilePatch(diffs); + patch: PropTypes.object.isRequired, } // TODO: should this function go in the Container since we aren't using an `item` for this component tree? @@ -31,7 +20,7 @@ export default class PullRequestChangedFilesController extends React.Component { render() { return ( From 63e3cfa81280c8764179728ce480469a4c43a241 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 13:01:47 -0800 Subject: [PATCH 024/102] fix derp merge --- lib/containers/pr-changed-files-container.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 6b63b3244a..ef5d7663a5 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -70,24 +70,11 @@ export default class PullRequestChangedFilesContainer extends React.Component { return
{this.state.error}
; } -<<<<<<< Updated upstream - return ; -||||||| merged common ancestors - return ( - - ); -======= return ( ); ->>>>>>> Stashed changes } } From bb2ab963c0ebd170e13bc869d3e335bd6384debf Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 12 Dec 2018 16:06:59 -0500 Subject: [PATCH 025/102] PrDetailView tests :white_check_mark: and :100: --- test/views/pr-detail-view.test.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/views/pr-detail-view.test.js b/test/views/pr-detail-view.test.js index d2c4ff5cf7..1b3a259c02 100644 --- a/test/views/pr-detail-view.test.js +++ b/test/views/pr-detail-view.test.js @@ -78,7 +78,7 @@ describe('PullRequestDetailView', function() { assert.lengthOf(wrapper.find(TabList), 1); const tabs = wrapper.find(Tab).getElements(); - assert.lengthOf(tabs, 3); + assert.lengthOf(tabs, 4); const tab0Children = tabs[0].props.children; assert.deepEqual(tab0Children[0].props, {icon: 'info', className: 'github-IssueishDetailView-tab-icon'}); @@ -92,7 +92,11 @@ describe('PullRequestDetailView', function() { assert.deepEqual(tab2Children[0].props, {icon: 'git-commit', className: 'github-IssueishDetailView-tab-icon'}); assert.deepEqual(tab2Children[1], 'Commits'); - assert.lengthOf(wrapper.find(TabPanel), 3); + const tab3Children = tabs[3].props.children; + assert.deepEqual(tab3Children[0].props, {icon: 'diff', className: 'github-IssueishDetailView-tab-icon'}); + assert.deepEqual(tab3Children[1], 'Files Changed'); + + assert.lengthOf(wrapper.find(TabPanel), 4); }); it('tells its tabs when the pull request is currently checked out', function() { From ae98c0e78b1c3690326eed742a7bb41aa6ae7fdf Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 15:22:18 -0800 Subject: [PATCH 026/102] add tests for `PullRequestChangedFilesController` --- .../pr-changed-file-controller.test.js | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 test/controllers/pr-changed-file-controller.test.js diff --git a/test/controllers/pr-changed-file-controller.test.js b/test/controllers/pr-changed-file-controller.test.js new file mode 100644 index 0000000000..b72b2e4ad5 --- /dev/null +++ b/test/controllers/pr-changed-file-controller.test.js @@ -0,0 +1,35 @@ +import React from 'react'; +import {shallow} from 'enzyme'; + +import {multiFilePatchBuilder} from '../builder/patch'; + +import PullRequestChangedFilesController from '../../lib/controllers/pr-changed-files-controller'; + +const {multiFilePatch} = multiFilePatchBuilder() + .addFilePatch(fp => { + fp.addHunk(h => { + h.unchanged('line 0', 'line-1').added('added line').unchanged('line 2'); + }); + }) + .build(); + +describe('PullRequestChangedFilesController', function() { + + function buildApp(overrideProps = {}) { + return ( + + ); + } + + it('passes child props through to MultiFilePatchView', function() { + const extraProp = Symbol('so extra you wont believe it'); + + const wrapper = shallow(buildApp({extraProp})); + + const controller = wrapper.find('PullRequestChangedFilesView'); + assert.strictEqual(controller.prop('extraProp'), extraProp); + }); +}); From db3f189b64c09e786a8e05492ef32279f9360f0e Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 15:26:12 -0800 Subject: [PATCH 027/102] wip -- why diff fixture no worky? --- lib/containers/pr-changed-files-container.js | 1 + .../pr-changed-files-container.test.js | 23 +++++++++++++++---- test/fixtures/diffs/patch.js | 13 +++++++++++ 3 files changed, 32 insertions(+), 5 deletions(-) create mode 100644 test/fixtures/diffs/patch.js diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index ef5d7663a5..703aa33d7a 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -38,6 +38,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { } buildPatch(rawDiff) { + console.log('!!!! rawDiff', rawDiff); const diffs = parseDiff(rawDiff); return buildMultiFilePatch(diffs); } diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index 603e1d8540..53605f1952 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -1,9 +1,14 @@ import React from 'react'; import {shallow} from 'enzyme'; +import {multiFilePatchBuilder} from '../builder/patch'; +import patch from '../fixtures/diffs/patch'; import PullRequestChangedFilesContainer from '../../lib/containers/pr-changed-files-container'; +import IssueishDetailItem from '../../lib/items/issueish-detail-item'; -describe('PullRequestChangedFilesContainer', function() { + + +describe.only('PullRequestChangedFilesContainer', function() { let diffResponse; function buildApp(overrideProps = {}) { @@ -14,6 +19,7 @@ describe('PullRequestChangedFilesContainer', function() { number={1804} token="1234" host="github.com" + itemType={IssueishDetailItem} {...overrideProps} /> ); @@ -27,7 +33,15 @@ describe('PullRequestChangedFilesContainer', function() { } beforeEach(function() { - setDiffResponse('default'); + const multiFilePatch = multiFilePatchBuilder() + .addFilePatch(fp => { + fp.addHunk(h => { + h.unchanged('line 0', 'line-1').added('added line').unchanged('line 2'); + }); + }) + .build(); + console.log('!!!! PATCH', patch); + setDiffResponse(patch); sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); }); @@ -36,7 +50,7 @@ describe('PullRequestChangedFilesContainer', function() { assert.isTrue(wrapper.find('LoadingView').exists()); }); - it('passes extra props through to PullRequestChangedFilesController', async function() { + it.only('passes extra props through to PullRequestChangedFilesController', async function() { const extraProp = Symbol('really really extra'); const wrapper = shallow(buildApp({extraProp})); @@ -67,14 +81,13 @@ describe('PullRequestChangedFilesContainer', function() { }); it('passes loaded diff data through to the controller', async function() { - setDiffResponse('some really swell diff'); const wrapper = shallow(buildApp({ token: '4321', })); await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); const controller = wrapper.find('PullRequestChangedFilesController'); - assert.strictEqual(controller.prop('diff'), '"some really swell diff"'); + assert.strictEqual(controller.prop('patch'), patch); assert.deepEqual(window.fetch.lastCall.args, [ 'https://api.github.com/repos/atom/github/pulls/1804', diff --git a/test/fixtures/diffs/patch.js b/test/fixtures/diffs/patch.js new file mode 100644 index 0000000000..7f20ff417e --- /dev/null +++ b/test/fixtures/diffs/patch.js @@ -0,0 +1,13 @@ +import dedent from 'dedent-js'; +const patch = ` + diff --git file.txt file.txt + index 83db48f..bf269f4 100644 + --- file.txt + +++ file.txt + @@ -1,3 +1,3 @@ class Thing { + line1 + -line2 + +new line + line3 +`; +export default patch; From d4e2dca692c72d7d164e85a6d1f610b96fa0f83f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 12 Dec 2018 15:44:54 -0800 Subject: [PATCH 028/102] Dedent patch fixture Co-Authored-By: Tilde Ann Thurium Co-Authored-By: Ashi Krishnan --- test/fixtures/diffs/patch.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/diffs/patch.js b/test/fixtures/diffs/patch.js index 7f20ff417e..ba9c32c6ef 100644 --- a/test/fixtures/diffs/patch.js +++ b/test/fixtures/diffs/patch.js @@ -1,5 +1,5 @@ import dedent from 'dedent-js'; -const patch = ` +const patch = dedent` diff --git file.txt file.txt index 83db48f..bf269f4 100644 --- file.txt From 3df90c5d6cc3ad3854e769375a28be0cb3bcb36c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 12 Dec 2018 15:45:54 -0800 Subject: [PATCH 029/102] Don't JSON stringify stubbed response Co-Authored-By: Tilde Ann Thurium Co-Authored-By: Ashi Krishnan --- test/containers/pr-changed-files-container.test.js | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index 53605f1952..b10ec92f80 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -7,7 +7,6 @@ import PullRequestChangedFilesContainer from '../../lib/containers/pr-changed-fi import IssueishDetailItem from '../../lib/items/issueish-detail-item'; - describe.only('PullRequestChangedFilesContainer', function() { let diffResponse; @@ -26,21 +25,13 @@ describe.only('PullRequestChangedFilesContainer', function() { } function setDiffResponse(body) { - diffResponse = new window.Response(JSON.stringify(body), { + diffResponse = new window.Response(body, { status: 200, headers: {'Content-type': 'text/plain'}, }); } beforeEach(function() { - const multiFilePatch = multiFilePatchBuilder() - .addFilePatch(fp => { - fp.addHunk(h => { - h.unchanged('line 0', 'line-1').added('added line').unchanged('line 2'); - }); - }) - .build(); - console.log('!!!! PATCH', patch); setDiffResponse(patch); sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); }); From 9c69216ed0a53031fadb4cfa607363833394f4a5 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 15:53:53 -0800 Subject: [PATCH 030/102] naming things is hard, let's go shopping --- test/containers/pr-changed-files-container.test.js | 7 +++---- test/fixtures/diffs/{patch.js => raw-diff.js} | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) rename test/fixtures/diffs/{patch.js => raw-diff.js} (80%) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index b10ec92f80..ab76c98999 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -1,8 +1,7 @@ import React from 'react'; import {shallow} from 'enzyme'; -import {multiFilePatchBuilder} from '../builder/patch'; -import patch from '../fixtures/diffs/patch'; +import rawDiff from '../fixtures/diffs/raw-diff'; import PullRequestChangedFilesContainer from '../../lib/containers/pr-changed-files-container'; import IssueishDetailItem from '../../lib/items/issueish-detail-item'; @@ -32,7 +31,7 @@ describe.only('PullRequestChangedFilesContainer', function() { } beforeEach(function() { - setDiffResponse(patch); + setDiffResponse(rawDiff); sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); }); @@ -41,7 +40,7 @@ describe.only('PullRequestChangedFilesContainer', function() { assert.isTrue(wrapper.find('LoadingView').exists()); }); - it.only('passes extra props through to PullRequestChangedFilesController', async function() { + it('passes extra props through to PullRequestChangedFilesController', async function() { const extraProp = Symbol('really really extra'); const wrapper = shallow(buildApp({extraProp})); diff --git a/test/fixtures/diffs/patch.js b/test/fixtures/diffs/raw-diff.js similarity index 80% rename from test/fixtures/diffs/patch.js rename to test/fixtures/diffs/raw-diff.js index ba9c32c6ef..81f56d896f 100644 --- a/test/fixtures/diffs/patch.js +++ b/test/fixtures/diffs/raw-diff.js @@ -1,5 +1,5 @@ import dedent from 'dedent-js'; -const patch = dedent` +const rawDiff = dedent` diff --git file.txt file.txt index 83db48f..bf269f4 100644 --- file.txt @@ -10,4 +10,4 @@ const patch = dedent` +new line line3 `; -export default patch; +export default rawDiff; From 206c3523d3d064dd2950775969c4b8b0dcfea8e7 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 15:55:36 -0800 Subject: [PATCH 031/102] `itemType` test is now unnecessary --- lib/containers/pr-changed-files-container.js | 1 - test/containers/pr-changed-files-container.test.js | 8 -------- 2 files changed, 9 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 703aa33d7a..ef5d7663a5 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -38,7 +38,6 @@ export default class PullRequestChangedFilesContainer extends React.Component { } buildPatch(rawDiff) { - console.log('!!!! rawDiff', rawDiff); const diffs = parseDiff(rawDiff); return buildMultiFilePatch(diffs); } diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index ab76c98999..d91134fc50 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -50,14 +50,6 @@ describe.only('PullRequestChangedFilesContainer', function() { assert.strictEqual(controller.prop('extraProp'), extraProp); }); - it('passes itemType prop to PullRequestChangedFilesController', async function() { - const wrapper = shallow(buildApp()); - await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); - - const controller = wrapper.find('PullRequestChangedFilesController'); - assert.strictEqual(controller.prop('itemType'), PullRequestChangedFilesContainer); - }); - it('builds the diff URL', function() { const wrapper = shallow(buildApp({ owner: 'smashwilson', From 924113b749c1bcadd5ea8ba1272a8397dcde29bf Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 15:59:01 -0800 Subject: [PATCH 032/102] rename `patch` to `multiFilePatch` ...like Angela Davis, I'm into specificity. --- lib/containers/pr-changed-files-container.js | 6 +++--- lib/controllers/pr-changed-files-controller.js | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index ef5d7663a5..ac561f03f3 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -53,8 +53,8 @@ export default class PullRequestChangedFilesContainer extends React.Component { }); if (response.ok) { const rawDiff = await response.text(); - const patch = this.buildPatch(rawDiff); - await new Promise(resolve => this.setState({isLoading: false, patch}, resolve)); + const multiFilePatch = this.buildPatch(rawDiff); + await new Promise(resolve => this.setState({isLoading: false, multiFilePatch}, resolve)); } else { // TODO: make error messages more user friendly / readable await new Promise(resolve => this.setState({isLoading: false, error: response.statusText}, resolve)); @@ -72,7 +72,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { return ( ); diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index e718c47942..ecc0ec98e6 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -5,7 +5,7 @@ import PullRequestChangedFilesView from '../views/pr-changed-files-view'; export default class PullRequestChangedFilesController extends React.Component { static propTypes = { - patch: PropTypes.object.isRequired, + multiFilePatch: PropTypes.object.isRequired, } // TODO: should this function go in the Container since we aren't using an `item` for this component tree? @@ -20,7 +20,6 @@ export default class PullRequestChangedFilesController extends React.Component { render() { return ( From 5aa73c1da4e684517c1a761f06e7aed6cb81d9fc Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 16:13:09 -0800 Subject: [PATCH 033/102] fix multiFilePatch prop test in PullRequestChangedFilesController --- test/containers/pr-changed-files-container.test.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index d91134fc50..c3afaf423f 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -1,12 +1,16 @@ import React from 'react'; import {shallow} from 'enzyme'; +import {parse as parseDiff} from 'what-the-diff'; + import rawDiff from '../fixtures/diffs/raw-diff'; +import {buildMultiFilePatch} from '../../lib/models/patch'; + import PullRequestChangedFilesContainer from '../../lib/containers/pr-changed-files-container'; import IssueishDetailItem from '../../lib/items/issueish-detail-item'; -describe.only('PullRequestChangedFilesContainer', function() { +describe('PullRequestChangedFilesContainer', function() { let diffResponse; function buildApp(overrideProps = {}) { @@ -69,7 +73,11 @@ describe.only('PullRequestChangedFilesContainer', function() { await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); const controller = wrapper.find('PullRequestChangedFilesController'); - assert.strictEqual(controller.prop('patch'), patch); + // comparing the whole multiFilePatch is annoying because there are hashes + // that are generated that will always be different when a new mfp is built. + // just verify the filePatches are the same and move on with our lives. + const expectedFilePatches = buildMultiFilePatch(parseDiff(rawDiff)).filePatches; + assert.deepEqual(controller.prop('multiFilePatch').filePatches, expectedFilePatches); assert.deepEqual(window.fetch.lastCall.args, [ 'https://api.github.com/repos/atom/github/pulls/1804', From 31c744c3d3ac7da22ab796f7909473ddfe86403d Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 16:13:43 -0800 Subject: [PATCH 034/102] pluralization is hard ok --- ...ile-controller.test.js => pr-changed-files-controller.test.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/controllers/{pr-changed-file-controller.test.js => pr-changed-files-controller.test.js} (100%) diff --git a/test/controllers/pr-changed-file-controller.test.js b/test/controllers/pr-changed-files-controller.test.js similarity index 100% rename from test/controllers/pr-changed-file-controller.test.js rename to test/controllers/pr-changed-files-controller.test.js From f149919613498cb2ea17a951343affb79cd693ca Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 16:21:56 -0800 Subject: [PATCH 035/102] fix missing prop errors in test --- test/controllers/pr-changed-files-controller.test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/controllers/pr-changed-files-controller.test.js b/test/controllers/pr-changed-files-controller.test.js index b72b2e4ad5..370471947c 100644 --- a/test/controllers/pr-changed-files-controller.test.js +++ b/test/controllers/pr-changed-files-controller.test.js @@ -18,7 +18,8 @@ describe('PullRequestChangedFilesController', function() { function buildApp(overrideProps = {}) { return ( ); @@ -26,10 +27,10 @@ describe('PullRequestChangedFilesController', function() { it('passes child props through to MultiFilePatchView', function() { const extraProp = Symbol('so extra you wont believe it'); - const wrapper = shallow(buildApp({extraProp})); const controller = wrapper.find('PullRequestChangedFilesView'); assert.strictEqual(controller.prop('extraProp'), extraProp); }); + }); From 44f5fd125d9a224b595530507bcdf7c552aecbb2 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 16:34:37 -0800 Subject: [PATCH 036/102] move `destroy` method to `IssueishDetailItem` --- lib/containers/issueish-detail-container.js | 2 ++ lib/containers/pr-changed-files-container.js | 3 +++ lib/controllers/issueish-detail-controller.js | 2 ++ lib/controllers/pr-changed-files-controller.js | 10 ---------- lib/items/issueish-detail-item.js | 9 +++++++++ lib/views/pr-detail-view.js | 2 ++ 6 files changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/containers/issueish-detail-container.js b/lib/containers/issueish-detail-container.js index 5f8e12a147..fce0b4e7a9 100644 --- a/lib/containers/issueish-detail-container.js +++ b/lib/containers/issueish-detail-container.js @@ -36,6 +36,7 @@ export default class IssueishDetailContainer extends React.Component { // Action methods switchToIssueish: PropTypes.func.isRequired, onTitleChange: PropTypes.func.isRequired, + destroy: PropTypes.func.isRequired, // Item context itemType: ItemTypePropType.isRequired, @@ -195,6 +196,7 @@ export default class IssueishDetailContainer extends React.Component { config={this.props.config} itemType={this.props.itemType} + destroy={this.props.destroy} /> ); } diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index ac561f03f3..cd4d053007 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -20,6 +20,9 @@ export default class PullRequestChangedFilesContainer extends React.Component { // Item context itemType: ItemTypePropType.isRequired, + + // action methods + destroy: PropTypes.func.isRequired, } constructor(props) { diff --git a/lib/controllers/issueish-detail-controller.js b/lib/controllers/issueish-detail-controller.js index 0975c855ed..44e0b5c32c 100644 --- a/lib/controllers/issueish-detail-controller.js +++ b/lib/controllers/issueish-detail-controller.js @@ -52,6 +52,7 @@ export class BareIssueishDetailController extends React.Component { addRemote: PropTypes.func.isRequired, onTitleChange: PropTypes.func.isRequired, switchToIssueish: PropTypes.func.isRequired, + destroy: PropTypes.func.isRequired, // Item context itemType: ItemTypePropType.isRequired, @@ -142,6 +143,7 @@ export class BareIssueishDetailController extends React.Component { openCommit={this.openCommit} itemType={this.props.itemType} + destroy={this.props.destroy} /> ); } else { diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js index ecc0ec98e6..7a409ef7e7 100644 --- a/lib/controllers/pr-changed-files-controller.js +++ b/lib/controllers/pr-changed-files-controller.js @@ -8,19 +8,9 @@ export default class PullRequestChangedFilesController extends React.Component { multiFilePatch: PropTypes.object.isRequired, } - // TODO: should this function go in the Container since we aren't using an `item` for this component tree? - destroy = () => { - /* istanbul ignore else */ - // if (!this.isDestroyed) { - // this.emitter.emit('did-destroy'); - // this.isDestroyed = true; - // } - } - render() { return ( ); diff --git a/lib/items/issueish-detail-item.js b/lib/items/issueish-detail-item.js index 283c6535f9..cacc10c2c7 100644 --- a/lib/items/issueish-detail-item.js +++ b/lib/items/issueish-detail-item.js @@ -86,6 +86,7 @@ export default class IssueishDetailItem extends Component { tooltips={this.props.tooltips} config={this.props.config} + destroy={this.destroy} itemType={this.constructor} /> ); @@ -152,6 +153,14 @@ export default class IssueishDetailItem extends Component { return this.emitter.on('did-terminate-pending-state', callback); } + destroy = () => { + /* istanbul ignore else */ + if (!this.isDestroyed) { + this.emitter.emit('did-destroy'); + this.isDestroyed = true; + } + } + onDidDestroy(callback) { return this.emitter.on('did-destroy', callback); } diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 637e5aaaf1..2e7b771a72 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -94,6 +94,7 @@ export class BarePullRequestDetailView extends React.Component { // Action functions openCommit: PropTypes.func.isRequired, + destroy: PropTypes.func.isRequired, // Item context itemType: ItemTypePropType.isRequired, @@ -211,6 +212,7 @@ export class BarePullRequestDetailView extends React.Component { config={this.props.config} itemType={this.props.itemType} + destroy={this.props.destroy} />
From 00232d1f5fb75fb1358016fb754bea15d47f5f6c Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Wed, 12 Dec 2018 17:17:19 -0800 Subject: [PATCH 037/102] :fire: PullRequestChangedFiles controller and view they're not doing shit, dawg. We can revert this if there ends up being some use for them. --- lib/containers/pr-changed-files-container.js | 15 ++++++-- .../pr-changed-files-controller.js | 18 ---------- lib/views/pr-changed-files-view.js | 20 ----------- .../pr-changed-files-controller.test.js | 36 ------------------- 4 files changed, 13 insertions(+), 76 deletions(-) delete mode 100644 lib/controllers/pr-changed-files-controller.js delete mode 100644 lib/views/pr-changed-files-view.js delete mode 100644 test/controllers/pr-changed-files-controller.test.js diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index cd4d053007..de7fb2f599 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -3,7 +3,7 @@ import PropTypes from 'prop-types'; import {parse as parseDiff} from 'what-the-diff'; import {ItemTypePropType} from '../prop-types'; -import PullRequestChangedFilesController from '../controllers/pr-changed-files-controller'; +import MultiFilePatchController from '../controllers/multi-file-patch-controller'; import LoadingView from '../views/loading-view'; import {buildMultiFilePatch} from '../models/patch'; @@ -23,6 +23,16 @@ export default class PullRequestChangedFilesContainer extends React.Component { // action methods destroy: PropTypes.func.isRequired, + + // Atom environment + workspace: PropTypes.object.isRequired, + commands: PropTypes.object.isRequired, + keymaps: PropTypes.object.isRequired, + tooltips: PropTypes.object.isRequired, + config: PropTypes.object.isRequired, + + // local repo as opposed to pull request repo + localRepository: PropTypes.object.isRequired, } constructor(props) { @@ -74,8 +84,9 @@ export default class PullRequestChangedFilesContainer extends React.Component { } return ( - ); diff --git a/lib/controllers/pr-changed-files-controller.js b/lib/controllers/pr-changed-files-controller.js deleted file mode 100644 index 7a409ef7e7..0000000000 --- a/lib/controllers/pr-changed-files-controller.js +++ /dev/null @@ -1,18 +0,0 @@ -import React from 'react'; -import PropTypes from 'prop-types'; - -import PullRequestChangedFilesView from '../views/pr-changed-files-view'; - -export default class PullRequestChangedFilesController extends React.Component { - static propTypes = { - multiFilePatch: PropTypes.object.isRequired, - } - - render() { - return ( - - ); - } -} diff --git a/lib/views/pr-changed-files-view.js b/lib/views/pr-changed-files-view.js deleted file mode 100644 index f34a0114d2..0000000000 --- a/lib/views/pr-changed-files-view.js +++ /dev/null @@ -1,20 +0,0 @@ -import React from 'react'; -import PropTypes from 'prop-types'; - -import MultiFilePatchController from '../controllers/multi-file-patch-controller'; - -export default class PullRequestChangedFilesView extends React.Component { - static propTypes = { - localRepository: PropTypes.object.isRequired, - multiFilePatch: PropTypes.object.isRequired, - } - - render() { - return ( - ); - } -} diff --git a/test/controllers/pr-changed-files-controller.test.js b/test/controllers/pr-changed-files-controller.test.js deleted file mode 100644 index 370471947c..0000000000 --- a/test/controllers/pr-changed-files-controller.test.js +++ /dev/null @@ -1,36 +0,0 @@ -import React from 'react'; -import {shallow} from 'enzyme'; - -import {multiFilePatchBuilder} from '../builder/patch'; - -import PullRequestChangedFilesController from '../../lib/controllers/pr-changed-files-controller'; - -const {multiFilePatch} = multiFilePatchBuilder() - .addFilePatch(fp => { - fp.addHunk(h => { - h.unchanged('line 0', 'line-1').added('added line').unchanged('line 2'); - }); - }) - .build(); - -describe('PullRequestChangedFilesController', function() { - - function buildApp(overrideProps = {}) { - return ( - - ); - } - - it('passes child props through to MultiFilePatchView', function() { - const extraProp = Symbol('so extra you wont believe it'); - const wrapper = shallow(buildApp({extraProp})); - - const controller = wrapper.find('PullRequestChangedFilesView'); - assert.strictEqual(controller.prop('extraProp'), extraProp); - }); - -}); From df729f906c05d589272a26dba050687c938bf619 Mon Sep 17 00:00:00 2001 From: simurai Date: Thu, 13 Dec 2018 15:29:23 +0900 Subject: [PATCH 038/102] Make header and nav full-width --- styles/issueish-detail-view.less | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/styles/issueish-detail-view.less b/styles/issueish-detail-view.less index 59306041a4..85b2948f7c 100644 --- a/styles/issueish-detail-view.less +++ b/styles/issueish-detail-view.less @@ -1,8 +1,7 @@ @import 'variables'; .github-IssueishDetailView { - padding: @component-padding * 3; - overflow: auto; + // overflow: auto; // TODO add somewhere else position: absolute; top: 0; right: 0; @@ -13,17 +12,15 @@ background-color: @base-background-color; &-container { - max-width: 48em; - margin: 0 auto; + // max-width: 48em; // TODO add somewhere else + // margin: 0 auto; // TODO add somewhere else } // Header ------------------------ &-header { - border: 1px solid @base-border-color; - border-radius: @component-border-radius; - margin-bottom: 20px; + border-bottom: 1px solid @base-border-color; } &-headerRow { @@ -140,23 +137,24 @@ &-tablist { display: flex; + justify-content: center; padding: 0; list-style: none; + border-bottom: 1px solid @base-border-color; + background-color: @app-background-color; } &-tab { - flex: 1; - padding: @component-padding/2 @component-padding; + padding: @component-padding @component-padding*1.5; text-align: center; - border: 1px solid transparent; - border-bottom-color: @base-border-color; - border-radius: @component-border-radius @component-border-radius 0 0; + font-size: @font-size; + font-weight: 600; + color: @text-color-subtle; cursor: default; &.react-tabs__tab--selected { - color: @text-color-highlight; - border-color: @base-border-color; - border-bottom-color: transparent; + color: @text-color-selected; + box-shadow: 0 1px 0 @button-background-color-selected; } } From 8a3ddd8705ef37712fe6b96c17e429cab487ba41 Mon Sep 17 00:00:00 2001 From: simurai Date: Thu, 13 Dec 2018 17:29:42 +0900 Subject: [PATCH 039/102] Add scrolling --- styles/issueish-detail-view.less | 107 ++++++++++++++++++------------- styles/pr-commit-view.less | 1 - 2 files changed, 62 insertions(+), 46 deletions(-) diff --git a/styles/issueish-detail-view.less b/styles/issueish-detail-view.less index 85b2948f7c..70a31bff88 100644 --- a/styles/issueish-detail-view.less +++ b/styles/issueish-detail-view.less @@ -1,7 +1,6 @@ @import 'variables'; .github-IssueishDetailView { - // overflow: auto; // TODO add somewhere else position: absolute; top: 0; right: 0; @@ -11,9 +10,24 @@ line-height: 1.5; background-color: @base-background-color; + + // Layout ------------------------ + &-container { - // max-width: 48em; // TODO add somewhere else - // margin: 0 auto; // TODO add somewhere else + display: flex; + flex-direction: column; + height: 100%; + } + + .react-tabs { + flex: 1; + display: flex; + flex-direction: column; + } + + .react-tabs__tab-panel--selected { + flex: 1; + overflow: auto; } @@ -104,23 +118,6 @@ } } - - // Reactions ------------------------ - - &-reactions { - padding-top: @component-padding*3; - border-top: 1px solid @base-border-color; - - &:empty { - display: none; - } - - &Group { - margin-right: @component-padding*2; - } - } - - // Meta ------------------------ &-meta { @@ -136,9 +133,11 @@ // Tabs ------------------------ &-tablist { + flex: none; display: flex; justify-content: center; padding: 0; + margin: 0; list-style: none; border-bottom: 1px solid @base-border-color; background-color: @app-background-color; @@ -165,45 +164,63 @@ } } - // Files Changed ------------------------ - // TODO: it would probably be better to use absolute positioning - // and then just set top / left / bottom right, and also to make - // the whole tab container responsive for bigger screens. - &-filesChanged { - height: 100vh; + // Footer ------------------------ + + &-footer { + padding: @component-padding; + text-align: center; + border-top: 1px solid @base-border-color; + background-color: @app-background-color; + + &Link.icon { + color: @text-color-subtle; + &:before { + vertical-align: -2px; + } + } } - // Build Status ------------------------ + // Tab Content ------------------------ - &-buildStatus { - margin: @component-padding*3 0; + .github-DotComMarkdownHtml, + &-reactions, + .github-PrTimeline { + max-width: 60em; + margin: 0 auto; + } + // Overview + .github-DotComMarkdownHtml { + padding: @component-padding*2; + } + &-reactions { + padding-top: @component-padding*3; + border-top: 1px solid @base-border-color; &:empty { display: none; } + &Group { + margin-right: @component-padding*2; + } } - - - // PR body ------------------------ - - &-container > .github-DotComMarkdownHtml { - margin: @component-padding*3 0; + .github-PrTimeline { + padding: @component-padding*2; } - // Footer ------------------------ - - &-footer { - margin-top: @component-padding * 3; - text-align: center; - &Link.icon { - color: @text-color-subtle; - &:before { - vertical-align: -2px; - } + // Build Status + &-buildStatus { + padding: @component-padding; + &:empty { + display: none; } } + // Commits + .github-PrCommitsView-commitWrapper { + padding: @component-padding; + } + } diff --git a/styles/pr-commit-view.less b/styles/pr-commit-view.less index 0744181821..cd4b028fde 100644 --- a/styles/pr-commit-view.less +++ b/styles/pr-commit-view.less @@ -22,7 +22,6 @@ border-bottom: none; &:first-child { - margin-top: @default-padding * 2; border-top-left-radius: @component-border-radius; border-top-right-radius: @component-border-radius; } From ac5de8e4a5b1558c1d4682078c8619c856c8d634 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 14:22:48 +0100 Subject: [PATCH 040/102] refetch diff on refresh --- lib/containers/pr-changed-files-container.js | 10 ++++++++++ lib/views/pr-detail-view.js | 2 ++ 2 files changed, 12 insertions(+) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index de7fb2f599..83a1e13b00 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -33,6 +33,9 @@ export default class PullRequestChangedFilesContainer extends React.Component { // local repo as opposed to pull request repo localRepository: PropTypes.object.isRequired, + + // refetch diff on refresh + shouldRefetch: PropTypes.bool.isRequired, } constructor(props) { @@ -41,6 +44,13 @@ export default class PullRequestChangedFilesContainer extends React.Component { this.fetchDiff(); } + componentDidUpdate(prevProps) { + if (this.props.shouldRefetch && !prevProps.shouldRefetch) { + this.setState({isLoading: true}); + this.fetchDiff(); + } + } + // Generate a v3 GitHub API REST URL for the pull request resource. // Example: https://api.github.com/repos/atom/github/pulls/1829 getDiffURL() { diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 2e7b771a72..a9987d22b1 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -213,6 +213,8 @@ export class BarePullRequestDetailView extends React.Component { itemType={this.props.itemType} destroy={this.props.destroy} + + shouldRefetch={this.state.refreshing} />
From 6b2590b807f28893a28eab14782a820f550268c6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 08:46:48 -0500 Subject: [PATCH 041/102] Endpoint model to derive GitHub API endpoints from a domain --- lib/models/endpoint.js | 40 +++++++++++++++++++++++++++++++++ test/models/endpoint.test.js | 43 ++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 lib/models/endpoint.js create mode 100644 test/models/endpoint.test.js diff --git a/lib/models/endpoint.js b/lib/models/endpoint.js new file mode 100644 index 0000000000..76e6e50ac1 --- /dev/null +++ b/lib/models/endpoint.js @@ -0,0 +1,40 @@ +function buildRestURI(endpoint, parts) { + const suffix = parts.map(encodeURIComponent).join('/'); + return `${endpoint.getRestRoot()}/${suffix}`; +} + +// API endpoint for GitHub.com +const dotcomEndpoint = { + getGraphQLRoot() { return 'https://api.github.com/graphql'; }, + + getRestRoot() { return 'https://api.github.com'; }, + + getRestURI(...parts) { return buildRestURI(this, parts); }, +}; + +// API endpoint for a GitHub enterprise installation. +class EnterpriseEndpoint { + constructor(domain) { + this.apiRoot = `https://${domain}/api/v3`; + } + + getGraphQLRoot() { + return `${this.apiRoot}/graphql`; + } + + getRestRoot() { + return this.apiRoot; + } + + getRestURI(...parts) { + return buildRestURI(this, parts); + } +} + +export function getEndpoint(host) { + if (host === 'github.com') { + return dotcomEndpoint; + } else { + return new EnterpriseEndpoint(host); + } +} diff --git a/test/models/endpoint.test.js b/test/models/endpoint.test.js new file mode 100644 index 0000000000..b57cc26e33 --- /dev/null +++ b/test/models/endpoint.test.js @@ -0,0 +1,43 @@ +import {getEndpoint} from '../../lib/models/endpoint'; + +describe('Endpoint', function() { + describe('on dotcom', function() { + let dotcom; + + beforeEach(function() { + dotcom = getEndpoint('github.com'); + }); + + it('identifies the GraphQL resource URI', function() { + assert.strictEqual(dotcom.getGraphQLRoot(), 'https://api.github.com/graphql'); + }); + + it('identifies the REST base resource URI', function() { + assert.strictEqual(dotcom.getRestRoot(), 'https://api.github.com'); + }); + + it('joins additional path segments to a REST URI', function() { + assert.strictEqual(dotcom.getRestURI('sub', 're?source'), 'https://api.github.com/sub/re%3Fsource'); + }); + }); + + describe('an enterprise instance', function() { + let enterprise; + + beforeEach(function() { + enterprise = getEndpoint('github.horse'); + }); + + it('identifies the GraphQL resource URI', function() { + assert.strictEqual(enterprise.getGraphQLRoot(), 'https://github.horse/api/v3/graphql'); + }); + + it('identifies the REST base resource URI', function() { + assert.strictEqual(enterprise.getRestRoot(), 'https://github.horse/api/v3'); + }); + + it('joins additional path segments to the REST URI', function() { + assert.strictEqual(enterprise.getRestURI('sub', 're?source'), 'https://github.horse/api/v3/sub/re%3Fsource'); + }); + }); +}); From f639ddb0ffc57794bba72848c20b5511c7f1a332 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 09:15:10 -0500 Subject: [PATCH 042/102] Access an Endpoint from a Remote --- lib/models/remote.js | 12 +++++++++++- test/models/remote.test.js | 13 +++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/models/remote.js b/lib/models/remote.js index d9a6248131..a5a441b8db 100644 --- a/lib/models/remote.js +++ b/lib/models/remote.js @@ -1,3 +1,5 @@ +import {getEndpoint} from './endpoint'; + export default class Remote { constructor(name, url) { this.name = name; @@ -39,7 +41,7 @@ export default class Remote { return this.repo; } - getNameOr(fallback) { + getNameOr() { return this.getName(); } @@ -51,6 +53,10 @@ export default class Remote { return `${this.owner}/${this.repo}`; } + getEndpoint() { + return this.domain === null ? null : getEndpoint(this.domain); + } + isPresent() { return true; } @@ -125,6 +131,10 @@ export const nullRemote = { return null; }, + getEndpoint() { + return null; + }, + isPresent() { return false; }, diff --git a/test/models/remote.test.js b/test/models/remote.test.js index 6d4535c7af..ce39859bc1 100644 --- a/test/models/remote.test.js +++ b/test/models/remote.test.js @@ -73,5 +73,18 @@ describe('Remote', function() { assert.isNull(nullRemote.getRepo()); assert.isNull(nullRemote.getSlug()); assert.strictEqual(nullRemote.getNameOr('else'), 'else'); + assert.isNull(nullRemote.getEndpoint()); + }); + + describe('getEndpoint', function() { + it('accesses an Endpoint for the corresponding GitHub host', function() { + const remote = new Remote('origin', 'git@github.com:atom/github.git'); + assert.strictEqual(remote.getEndpoint().getGraphQLRoot(), 'https://api.github.com/graphql'); + }); + + it('returns null for non-GitHub URLs', function() { + const elsewhere = new Remote('mirror', 'https://me@bitbucket.org/team/repo.git'); + assert.isNull(elsewhere.getEndpoint()); + }); }); }); From 35890808cbf364a21f52d40e6579bfd039b652fa Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 09:52:13 -0500 Subject: [PATCH 043/102] Access a login model key for an Endpoint Formed as protocol + hostname for consistency with the keys we're already storing. --- lib/models/endpoint.js | 37 ++++++++++++++++-------------------- test/models/endpoint.test.js | 8 ++++++++ 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/lib/models/endpoint.js b/lib/models/endpoint.js index 76e6e50ac1..0f5e444e61 100644 --- a/lib/models/endpoint.js +++ b/lib/models/endpoint.js @@ -1,40 +1,35 @@ -function buildRestURI(endpoint, parts) { - const suffix = parts.map(encodeURIComponent).join('/'); - return `${endpoint.getRestRoot()}/${suffix}`; -} - -// API endpoint for GitHub.com -const dotcomEndpoint = { - getGraphQLRoot() { return 'https://api.github.com/graphql'; }, - - getRestRoot() { return 'https://api.github.com'; }, - - getRestURI(...parts) { return buildRestURI(this, parts); }, -}; +// API endpoint for a GitHub instance, either dotcom or an Enterprise installation +class Endpoint { + constructor(domain, apiRoot) { + this.domain = domain; + this.apiRoot = apiRoot; + } -// API endpoint for a GitHub enterprise installation. -class EnterpriseEndpoint { - constructor(domain) { - this.apiRoot = `https://${domain}/api/v3`; + getRestURI(...parts) { + const suffix = parts.map(encodeURIComponent).join('/'); + return `${this.apiRoot}/${suffix}`; } getGraphQLRoot() { - return `${this.apiRoot}/graphql`; + return this.getRestURI('graphql'); } getRestRoot() { return this.apiRoot; } - getRestURI(...parts) { - return buildRestURI(this, parts); + getLoginAccount() { + return `https://${this.domain}`; } } +// API endpoint for GitHub.com +const dotcomEndpoint = new Endpoint('api.github.com', 'https://api.github.com'); + export function getEndpoint(host) { if (host === 'github.com') { return dotcomEndpoint; } else { - return new EnterpriseEndpoint(host); + return new Endpoint(host, `https://${host}/api/v3`); } } diff --git a/test/models/endpoint.test.js b/test/models/endpoint.test.js index b57cc26e33..d3823ac58d 100644 --- a/test/models/endpoint.test.js +++ b/test/models/endpoint.test.js @@ -19,6 +19,10 @@ describe('Endpoint', function() { it('joins additional path segments to a REST URI', function() { assert.strictEqual(dotcom.getRestURI('sub', 're?source'), 'https://api.github.com/sub/re%3Fsource'); }); + + it('accesses a login model account', function() { + assert.strictEqual(dotcom.getLoginAccount(), 'https://api.github.com'); + }); }); describe('an enterprise instance', function() { @@ -39,5 +43,9 @@ describe('Endpoint', function() { it('joins additional path segments to the REST URI', function() { assert.strictEqual(enterprise.getRestURI('sub', 're?source'), 'https://github.horse/api/v3/sub/re%3Fsource'); }); + + it('accesses a login model key', function() { + assert.strictEqual(enterprise.getLoginAccount(), 'https://github.horse'); + }); }); }); From abca1f5f910f43da54388491915b58c5a92ed0e1 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 10:49:17 -0500 Subject: [PATCH 044/102] Access an Endpoint's host, the user-facing GitHub domain --- lib/models/endpoint.js | 26 ++++++++++++++++---------- test/models/endpoint.test.js | 10 ++++++++++ 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/lib/models/endpoint.js b/lib/models/endpoint.js index 0f5e444e61..3fc5fd2fef 100644 --- a/lib/models/endpoint.js +++ b/lib/models/endpoint.js @@ -1,13 +1,14 @@ -// API endpoint for a GitHub instance, either dotcom or an Enterprise installation +// API endpoint for a GitHub instance, either dotcom or an Enterprise installation. class Endpoint { - constructor(domain, apiRoot) { - this.domain = domain; - this.apiRoot = apiRoot; + constructor(host, apiHost, apiRouteParts) { + this.host = host; + this.apiHost = apiHost; + this.apiRoute = apiRouteParts.map(encodeURIComponent).join('/'); } getRestURI(...parts) { - const suffix = parts.map(encodeURIComponent).join('/'); - return `${this.apiRoot}/${suffix}`; + const sep = parts.length > 0 ? '/' : ''; + return this.getRestRoot() + sep + parts.map(encodeURIComponent).join('/'); } getGraphQLRoot() { @@ -15,21 +16,26 @@ class Endpoint { } getRestRoot() { - return this.apiRoot; + const sep = this.apiRoute !== '' ? '/' : ''; + return `https://${this.apiHost}${sep}${this.apiRoute}`; + } + + getHost() { + return this.host; } getLoginAccount() { - return `https://${this.domain}`; + return `https://${this.apiHost}`; } } // API endpoint for GitHub.com -const dotcomEndpoint = new Endpoint('api.github.com', 'https://api.github.com'); +const dotcomEndpoint = new Endpoint('github.com', 'api.github.com', []); export function getEndpoint(host) { if (host === 'github.com') { return dotcomEndpoint; } else { - return new Endpoint(host, `https://${host}/api/v3`); + return new Endpoint(host, host, ['api', 'v3']); } } diff --git a/test/models/endpoint.test.js b/test/models/endpoint.test.js index d3823ac58d..33831ee4e6 100644 --- a/test/models/endpoint.test.js +++ b/test/models/endpoint.test.js @@ -14,12 +14,17 @@ describe('Endpoint', function() { it('identifies the REST base resource URI', function() { assert.strictEqual(dotcom.getRestRoot(), 'https://api.github.com'); + assert.strictEqual(dotcom.getRestURI(), 'https://api.github.com'); }); it('joins additional path segments to a REST URI', function() { assert.strictEqual(dotcom.getRestURI('sub', 're?source'), 'https://api.github.com/sub/re%3Fsource'); }); + it('accesses the hostname', function() { + assert.strictEqual(dotcom.getHost(), 'github.com'); + }); + it('accesses a login model account', function() { assert.strictEqual(dotcom.getLoginAccount(), 'https://api.github.com'); }); @@ -38,12 +43,17 @@ describe('Endpoint', function() { it('identifies the REST base resource URI', function() { assert.strictEqual(enterprise.getRestRoot(), 'https://github.horse/api/v3'); + assert.strictEqual(enterprise.getRestURI(), 'https://github.horse/api/v3'); }); it('joins additional path segments to the REST URI', function() { assert.strictEqual(enterprise.getRestURI('sub', 're?source'), 'https://github.horse/api/v3/sub/re%3Fsource'); }); + it('accesses the hostname', function() { + assert.strictEqual(enterprise.getHost(), 'github.horse'); + }); + it('accesses a login model key', function() { assert.strictEqual(enterprise.getLoginAccount(), 'https://github.horse'); }); From 792ee9ac98dd0e8cf46d2238cf8fd512ca2af813 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 10:50:14 -0500 Subject: [PATCH 045/102] Pass Endpoints instead of host strings --- .../current-pull-request-container.js | 15 +++++++-- lib/containers/issueish-detail-container.js | 20 ++++++------ lib/containers/issueish-search-container.js | 10 ++++-- lib/containers/pr-changed-files-container.js | 9 ++---- lib/containers/remote-container.js | 16 +++++----- lib/controllers/issueish-detail-controller.js | 6 ++-- .../issueish-searches-controller.js | 24 +++++++++----- lib/controllers/remote-controller.js | 17 ++++++---- lib/items/issueish-detail-item.js | 5 +-- lib/models/user-store.js | 6 ++-- lib/prop-types.js | 7 +++++ lib/relay-network-layer-manager.js | 31 +++++++++---------- lib/views/github-tab-view.js | 2 -- lib/views/pr-detail-view.js | 18 +++++------ .../current-pull-request-container.test.js | 2 +- .../issueish-search-container.test.js | 3 +- .../pr-changed-files-container.test.js | 24 ++++++-------- test/containers/remote-container.test.js | 3 +- .../issueish-searches-controller.test.js | 9 +++--- test/controllers/remote-controller.test.js | 5 +-- test/fixtures/props/issueish-pane-props.js | 3 +- test/items/issueish-detail-item.test.js | 4 +-- 22 files changed, 137 insertions(+), 102 deletions(-) diff --git a/lib/containers/current-pull-request-container.js b/lib/containers/current-pull-request-container.js index 1f012df810..9930f52180 100644 --- a/lib/containers/current-pull-request-container.js +++ b/lib/containers/current-pull-request-container.js @@ -4,13 +4,16 @@ import {QueryRenderer, graphql} from 'react-relay'; import {Disposable} from 'event-kit'; import {autobind} from '../helpers'; -import {RemotePropType, RemoteSetPropType, BranchSetPropType, OperationStateObserverPropType} from '../prop-types'; +import { + RemotePropType, RemoteSetPropType, BranchSetPropType, OperationStateObserverPropType, EndpointPropType, +} from '../prop-types'; import IssueishListController, {BareIssueishListController} from '../controllers/issueish-list-controller'; import CreatePullRequestTile from '../views/create-pull-request-tile'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; export default class CurrentPullRequestContainer extends React.Component { static propTypes = { + // Relay payload repository: PropTypes.shape({ id: PropTypes.string.isRequired, defaultBranchRef: PropTypes.shape({ @@ -19,9 +22,14 @@ export default class CurrentPullRequestContainer extends React.Component { }), }).isRequired, + // Connection + endpoint: EndpointPropType.isRequired, token: PropTypes.string.isRequired, - host: PropTypes.string.isRequired, + + // Search constraints limit: PropTypes.number, + + // Repository model attributes remoteOperationObserver: OperationStateObserverPropType.isRequired, remote: RemotePropType.isRequired, remotes: RemoteSetPropType.isRequired, @@ -29,6 +37,7 @@ export default class CurrentPullRequestContainer extends React.Component { aheadCount: PropTypes.number, pushInProgress: PropTypes.bool.isRequired, + // Actions onOpenIssueish: PropTypes.func.isRequired, onCreatePr: PropTypes.func.isRequired, } @@ -45,7 +54,7 @@ export default class CurrentPullRequestContainer extends React.Component { } render() { - const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.host, this.props.token); + const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.endpoint, this.props.token); const head = this.props.branches.getHeadBranch(); if (!head.isPresent()) { diff --git a/lib/containers/issueish-detail-container.js b/lib/containers/issueish-detail-container.js index fce0b4e7a9..c428b9fe47 100644 --- a/lib/containers/issueish-detail-container.js +++ b/lib/containers/issueish-detail-container.js @@ -3,21 +3,23 @@ import PropTypes from 'prop-types'; import yubikiri from 'yubikiri'; import {QueryRenderer, graphql} from 'react-relay'; +import {autobind} from '../helpers'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; -import {GithubLoginModelPropType, ItemTypePropType} from '../prop-types'; +import {GithubLoginModelPropType, ItemTypePropType, EndpointPropType} from '../prop-types'; import {UNAUTHENTICATED, INSUFFICIENT} from '../shared/keytar-strategy'; import GithubLoginView from '../views/github-login-view'; import LoadingView from '../views/loading-view'; import QueryErrorView from '../views/query-error-view'; import ObserveModel from '../views/observe-model'; -import IssueishDetailController from '../controllers/issueish-detail-controller'; import RelayEnvironment from '../views/relay-environment'; -import {autobind} from '../helpers'; +import IssueishDetailController from '../controllers/issueish-detail-controller'; export default class IssueishDetailContainer extends React.Component { static propTypes = { + // Connection + endpoint: EndpointPropType.isRequired, + // Issueish selection criteria - host: PropTypes.string, owner: PropTypes.string.isRequired, repo: PropTypes.string.isRequired, issueishNumber: PropTypes.number.isRequired, @@ -54,7 +56,7 @@ export default class IssueishDetailContainer extends React.Component { fetchToken(loginModel) { return yubikiri({ - token: loginModel.getToken(this.props.host), + token: loginModel.getToken(this.props.endpoint.getLoginAccount()), }); } @@ -109,7 +111,7 @@ export default class IssueishDetailContainer extends React.Component { return ; } - const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.host, token); + const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.endpoint, token); const query = graphql` query issueishDetailContainerQuery ( @@ -186,7 +188,7 @@ export default class IssueishDetailContainer extends React.Component { switchToIssueish={this.props.switchToIssueish} workdirPath={repository.getWorkingDirectoryPath()} - host={this.props.host} + endpoint={this.props.endpoint} token={token} workspace={this.props.workspace} @@ -202,10 +204,10 @@ export default class IssueishDetailContainer extends React.Component { } handleLogin(token) { - return this.props.loginModel.setToken(this.props.host, token); + return this.props.loginModel.setToken(this.props.endpoint.getLoginAccount(), token); } handleLogout() { - return this.props.loginModel.removeToken(this.props.host); + return this.props.loginModel.removeToken(this.props.endpoint.getLoginAccount()); } } diff --git a/lib/containers/issueish-search-container.js b/lib/containers/issueish-search-container.js index a937113658..4566ccec3e 100644 --- a/lib/containers/issueish-search-container.js +++ b/lib/containers/issueish-search-container.js @@ -4,18 +4,22 @@ import {QueryRenderer, graphql} from 'react-relay'; import {Disposable} from 'event-kit'; import {autobind} from '../helpers'; -import {SearchPropType, OperationStateObserverPropType} from '../prop-types'; +import {SearchPropType, OperationStateObserverPropType, EndpointPropType} from '../prop-types'; import IssueishListController, {BareIssueishListController} from '../controllers/issueish-list-controller'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; export default class IssueishSearchContainer extends React.Component { static propTypes = { + // Connection information + endpoint: EndpointPropType.isRequired, token: PropTypes.string.isRequired, - host: PropTypes.string.isRequired, + + // Search model limit: PropTypes.number, search: SearchPropType.isRequired, remoteOperationObserver: OperationStateObserverPropType.isRequired, + // Action methods onOpenIssueish: PropTypes.func.isRequired, onOpenSearch: PropTypes.func.isRequired, } @@ -32,7 +36,7 @@ export default class IssueishSearchContainer extends React.Component { } render() { - const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.host, this.props.token); + const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.endpoint, this.props.token); if (this.props.search.isNull()) { return ( diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 83a1e13b00..1ddfea6ad7 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -2,7 +2,7 @@ import React from 'react'; import PropTypes from 'prop-types'; import {parse as parseDiff} from 'what-the-diff'; -import {ItemTypePropType} from '../prop-types'; +import {ItemTypePropType, EndpointPropType} from '../prop-types'; import MultiFilePatchController from '../controllers/multi-file-patch-controller'; import LoadingView from '../views/loading-view'; import {buildMultiFilePatch} from '../models/patch'; @@ -15,8 +15,8 @@ export default class PullRequestChangedFilesContainer extends React.Component { number: PropTypes.number.isRequired, // Connection properties + endpoint: EndpointPropType.isRequired, token: PropTypes.string.isRequired, - host: PropTypes.string.isRequired, // Item context itemType: ItemTypePropType.isRequired, @@ -54,10 +54,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { // Generate a v3 GitHub API REST URL for the pull request resource. // Example: https://api.github.com/repos/atom/github/pulls/1829 getDiffURL() { - // TODO centralize endpoint translation logic between here and lib/relay-network-layer-manager.js. - // Maybe move it to the Remote model instead? - const endpoint = this.props.host === 'github.com' ? 'https://api.github.com' : this.props.host; - return `${endpoint}/repos/${this.props.owner}/${this.props.repo}/pulls/${this.props.number}`; + return this.props.endpoint.getRestURI('repos', this.props.owner, this.props.repo, 'pulls', this.props.number); } buildPatch(rawDiff) { diff --git a/lib/containers/remote-container.js b/lib/containers/remote-container.js index fc68447a1f..1ef7a37033 100644 --- a/lib/containers/remote-container.js +++ b/lib/containers/remote-container.js @@ -17,8 +17,6 @@ export default class RemoteContainer extends React.Component { static propTypes = { loginModel: PropTypes.object.isRequired, - host: PropTypes.string.isRequired, - remoteOperationObserver: OperationStateObserverPropType.isRequired, workingDirectory: PropTypes.string.isRequired, workspace: PropTypes.object.isRequired, @@ -39,7 +37,7 @@ export default class RemoteContainer extends React.Component { } fetchToken(loginModel) { - return loginModel.getToken(this.props.host); + return loginModel.getToken(this.endpoint().getLoginAccount()); } render() { @@ -69,7 +67,7 @@ export default class RemoteContainer extends React.Component { ); } - const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.host, token); + const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.endpoint(), token); const query = graphql` query remoteContainerQuery($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { @@ -114,7 +112,7 @@ export default class RemoteContainer extends React.Component { return ( { @@ -41,12 +43,9 @@ export function clearRelayExpectations() { responsesByQuery.clear(); } -const tokenPerURL = new Map(); -const fetchPerURL = new Map(); - function createFetchQuery(url) { if (atom.inSpecMode()) { - return function specFetchQuery(operation, variables, cacheConfig, uploadables) { + return function specFetchQuery(operation, variables, _cacheConfig, _uploadables) { const expectations = responsesByQuery.get(operation.name) || []; const match = expectations.find(expectation => { if (Object.keys(expectation.variables).length !== Object.keys(variables).length) { @@ -89,7 +88,7 @@ function createFetchQuery(url) { }; } - return async function fetchQuery(operation, variables, cacheConfig, uploadables) { + return async function fetchQuery(operation, variables, _cacheConfig, _uploadables) { const currentToken = tokenPerURL.get(url); const response = await fetch(url, { @@ -132,23 +131,23 @@ function createFetchQuery(url) { } export default class RelayNetworkLayerManager { - static getEnvironmentForHost(host, token) { - host = host === 'github.com' ? 'https://api.github.com' : host; - const url = host === 'https://api.github.com' ? `${host}/graphql` : `${host}/api/v3/graphql`; - let {environment, network} = relayEnvironmentPerGithubHost.get(host) || {}; - tokenPerURL.set(url, token); + static getEnvironmentForHost(endpoint, token) { + const url = endpoint.getGraphQLRoot(); + let {environment, network} = relayEnvironmentPerURL.get(url) || {}; + tokenPerURL.set(endpoint, token); if (!environment) { const source = new RecordSource(); const store = new Store(source); - network = Network.create(this.getFetchQuery(url, token)); + network = Network.create(this.getFetchQuery(endpoint, token)); environment = new Environment({network, store}); - relayEnvironmentPerGithubHost.set(host, {environment, network}); + relayEnvironmentPerURL.set(endpoint, {environment, network}); } return environment; } - static getFetchQuery(url, token) { + static getFetchQuery(endpoint, token) { + const url = endpoint.getGraphQLRoot(); tokenPerURL.set(url, token); let fetch = fetchPerURL.get(url); if (!fetch) { diff --git a/lib/views/github-tab-view.js b/lib/views/github-tab-view.js index 4c8bc73a4b..08dffb4763 100644 --- a/lib/views/github-tab-view.js +++ b/lib/views/github-tab-view.js @@ -47,10 +47,8 @@ export default class GitHubTabView extends React.Component { if (this.props.currentRemote.isPresent()) { // Single, chosen or unambiguous remote - // only supporting GH.com for now, hardcoded values return ( @@ -48,9 +47,9 @@ describe('PullRequestChangedFilesContainer', function() { const extraProp = Symbol('really really extra'); const wrapper = shallow(buildApp({extraProp})); - await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); + await assert.async.isTrue(wrapper.update().find('MultiFilePatchController').exists()); - const controller = wrapper.find('PullRequestChangedFilesController'); + const controller = wrapper.find('MultiFilePatchController'); assert.strictEqual(controller.prop('extraProp'), extraProp); }); @@ -59,7 +58,7 @@ describe('PullRequestChangedFilesContainer', function() { owner: 'smashwilson', repo: 'pushbot', number: 12, - host: 'github.com', + endpoint: getEndpoint('github.com'), })); const diffURL = wrapper.instance().getDiffURL(); @@ -70,14 +69,11 @@ describe('PullRequestChangedFilesContainer', function() { const wrapper = shallow(buildApp({ token: '4321', })); - await assert.async.isTrue(wrapper.update().find('PullRequestChangedFilesController').exists()); - - const controller = wrapper.find('PullRequestChangedFilesController'); - // comparing the whole multiFilePatch is annoying because there are hashes - // that are generated that will always be different when a new mfp is built. - // just verify the filePatches are the same and move on with our lives. - const expectedFilePatches = buildMultiFilePatch(parseDiff(rawDiff)).filePatches; - assert.deepEqual(controller.prop('multiFilePatch').filePatches, expectedFilePatches); + await assert.async.isTrue(wrapper.update().find('MultiFilePatchController').exists()); + + const controller = wrapper.find('MultiFilePatchController'); + const expected = buildMultiFilePatch(parseDiff(rawDiff)); + assert.isTrue(controller.prop('multiFilePatch').isEqual(expected)); assert.deepEqual(window.fetch.lastCall.args, [ 'https://api.github.com/repos/atom/github/pulls/1804', diff --git a/test/containers/remote-container.test.js b/test/containers/remote-container.test.js index 0f6cd34933..87c651a0c2 100644 --- a/test/containers/remote-container.test.js +++ b/test/containers/remote-container.test.js @@ -8,6 +8,7 @@ import Branch, {nullBranch} from '../../lib/models/branch'; import BranchSet from '../../lib/models/branch-set'; import GithubLoginModel from '../../lib/models/github-login-model'; import {nullOperationStateObserver} from '../../lib/models/operation-state-observer'; +import {getEndpoint} from '../../lib/models/endpoint'; import {InMemoryStrategy} from '../../lib/shared/keytar-strategy'; import RemoteContainer from '../../lib/containers/remote-container'; import {expectRelayQuery} from '../../lib/relay-network-layer-manager'; @@ -34,7 +35,7 @@ describe('RemoteContainer', function() { w.prop('search') === search); assert.isTrue(list.exists()); assert.strictEqual(list.prop('token'), '1234'); - assert.strictEqual(list.prop('host'), 'https://api.github.com'); + assert.strictEqual(list.prop('endpoint').getHost(), 'github.com'); } }); @@ -100,7 +101,7 @@ describe('IssueishSearchesController', function() { await container.prop('onOpenIssueish')(issueish); assert.isTrue( atomEnv.workspace.open.calledWith( - `atom-github://issueish/https%3A%2F%2Fapi.github.com/atom/github/123?workdir=${encodeURIComponent(__dirname)}`, + `atom-github://issueish/github.com/atom/github/123?workdir=${encodeURIComponent(__dirname)}`, {pending: true, searchAllPanes: true}, ), ); diff --git a/test/controllers/remote-controller.test.js b/test/controllers/remote-controller.test.js index 3d175cc0a5..87c188d368 100644 --- a/test/controllers/remote-controller.test.js +++ b/test/controllers/remote-controller.test.js @@ -5,6 +5,7 @@ import {shell} from 'electron'; import BranchSet from '../../lib/models/branch-set'; import Branch, {nullBranch} from '../../lib/models/branch'; import Remote from '../../lib/models/remote'; +import {getEndpoint} from '../../lib/models/endpoint'; import {nullOperationStateObserver} from '../../lib/models/operation-state-observer'; import RemoteController from '../../lib/controllers/remote-controller'; import * as reporterProxy from '../../lib/reporter-proxy'; @@ -30,7 +31,7 @@ describe('RemoteController', function() { return ( Date: Thu, 13 Dec 2018 11:33:27 -0500 Subject: [PATCH 046/102] Count PrDetailView tab openings --- lib/views/pr-detail-view.js | 12 +++++++- test/views/pr-detail-view.test.js | 46 ++++++++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index 822083bbae..bd9d9a8083 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -142,7 +142,7 @@ export class BarePullRequestDetailView extends React.Component { const onBranch = this.props.checkoutOp.why() === checkoutStates.CURRENT; return ( - + Overview @@ -329,6 +329,16 @@ export class BarePullRequestDetailView extends React.Component { addEvent('open-pull-request-in-browser', {package: 'github', component: this.constructor.name}); } + recordOpenTabEvent = ind => { + const eventName = [ + 'open-pr-tab-overview', + 'open-pr-tab-build-status', + 'open-pr-tab-commits', + 'open-pr-tab-files-changed', + ][ind]; + addEvent(eventName, {package: 'github', component: this.constructor.name}); + } + refresh = () => { if (this.state.refreshing) { return; diff --git a/test/views/pr-detail-view.test.js b/test/views/pr-detail-view.test.js index 1b3a259c02..da42b7c7c4 100644 --- a/test/views/pr-detail-view.test.js +++ b/test/views/pr-detail-view.test.js @@ -243,16 +243,18 @@ describe('PullRequestDetailView', function() { }); }); - describe('clicking link to view issueish link', function() { - it('records an event', function() { + describe('metrics', function() { + beforeEach(function() { + sinon.stub(reporterProxy, 'addEvent'); + }); + + it('records clicking the link to view an issueish', function() { const wrapper = shallow(buildApp({ repositoryName: 'repo', ownerLogin: 'user0', issueishNumber: 100, })); - sinon.stub(reporterProxy, 'addEvent'); - const link = wrapper.find('a.github-IssueishDetailView-headerLink'); assert.strictEqual(link.text(), 'user0/repo#100'); assert.strictEqual(link.prop('href'), 'https://github.com/user0/repo/pull/100'); @@ -260,5 +262,41 @@ describe('PullRequestDetailView', function() { assert.isTrue(reporterProxy.addEvent.calledWith('open-pull-request-in-browser', {package: 'github', component: 'BarePullRequestDetailView'})); }); + + it('records opening the Overview tab', function() { + const wrapper = shallow(buildApp()); + const ind = wrapper.find('Tab').map(t => t.children().last().text()).indexOf('Overview'); + + wrapper.find('Tabs').prop('onSelect')(ind); + + assert.isTrue(reporterProxy.addEvent.calledWith('open-pr-tab-overview', {package: 'github', component: 'BarePullRequestDetailView'})); + }); + + it('records opening the Build Status tab', function() { + const wrapper = shallow(buildApp()); + const ind = wrapper.find('Tab').map(t => t.children().last().text()).indexOf('Build Status'); + + wrapper.find('Tabs').prop('onSelect')(ind); + + assert.isTrue(reporterProxy.addEvent.calledWith('open-pr-tab-build-status', {package: 'github', component: 'BarePullRequestDetailView'})); + }); + + it('records opening the Commits tab', function() { + const wrapper = shallow(buildApp()); + const ind = wrapper.find('Tab').map(t => t.children().last().text()).indexOf('Commits'); + + wrapper.find('Tabs').prop('onSelect')(ind); + + assert.isTrue(reporterProxy.addEvent.calledWith('open-pr-tab-commits', {package: 'github', component: 'BarePullRequestDetailView'})); + }); + + it('records opening the "Files Changed" tab', function() { + const wrapper = shallow(buildApp()); + const ind = wrapper.find('Tab').map(t => t.children().last().text()).indexOf('Files Changed'); + + wrapper.find('Tabs').prop('onSelect')(ind); + + assert.isTrue(reporterProxy.addEvent.calledWith('open-pr-tab-files-changed', {package: 'github', component: 'BarePullRequestDetailView'})); + }); }); }); From 0ba6c631f25ed2f8e6e2e63b11a2a5f6279ac4cb Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 18:09:38 +0100 Subject: [PATCH 047/102] somewhat better error handling --- lib/containers/pr-changed-files-container.js | 33 +++++++++++--------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 1ddfea6ad7..ed8b2da8c6 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -62,22 +62,27 @@ export default class PullRequestChangedFilesContainer extends React.Component { return buildMultiFilePatch(diffs); } - // TODO: deal with when we need to refetch data async fetchDiff() { const url = this.getDiffURL(); - const response = await fetch(url, { - headers: { - Accept: 'application/vnd.github.v3.diff', - Authorization: `bearer ${this.props.token}`, - }, - }); - if (response.ok) { - const rawDiff = await response.text(); - const multiFilePatch = this.buildPatch(rawDiff); - await new Promise(resolve => this.setState({isLoading: false, multiFilePatch}, resolve)); - } else { - // TODO: make error messages more user friendly / readable - await new Promise(resolve => this.setState({isLoading: false, error: response.statusText}, resolve)); + try { + const response = await fetch(url, { + headers: { + Accept: 'application/vnd.github.v3.diff', + Authorization: `bearer ${this.props.token}`, + }, + }); + if (response && response.ok) { + const rawDiff = await response.text(); + const multiFilePatch = this.buildPatch(rawDiff); + await new Promise(resolve => this.setState({isLoading: false, multiFilePatch}, resolve)); + } else { + throw new Error(response.statusText); + } + } catch (e) { + // eslint-disable-next-line no-console + console.error(e); + await new Promise(resolve => + this.setState({isLoading: false, error: 'Unable to load diff for this PR.'}, resolve)); } } From ef27922eb6b0d2e5789ebfdff9d3c15318dac1c4 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 14:27:49 -0500 Subject: [PATCH 048/102] Yep that was totally me --- lib/views/file-patch-header-view.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/views/file-patch-header-view.js b/lib/views/file-patch-header-view.js index 6d0b6e3e04..eba8278094 100644 --- a/lib/views/file-patch-header-view.js +++ b/lib/views/file-patch-header-view.js @@ -5,7 +5,7 @@ import PropTypes from 'prop-types'; import cx from 'classnames'; import RefHolder from '../models/ref-holder'; -import PullRequestChangedFilesContainer from '../containers/pr-changed-files-container'; +import IssueishDetailItem from '../items/issueish-detail-item'; import ChangedFileItem from '../items/changed-file-item'; import CommitDetailItem from '../items/commit-detail-item'; import {ItemTypePropType} from '../prop-types'; @@ -74,7 +74,7 @@ export default class FilePatchHeaderView extends React.Component { } renderButtonGroup() { - if (this.props.itemType === CommitDetailItem || this.props.itemType === PullRequestChangedFilesContainer) { + if (this.props.itemType === CommitDetailItem || this.props.itemType === IssueishDetailItem) { return null; } else { return ( From 3080420fb2ab1f5e93759eed431684b001e15f9d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 14:31:14 -0500 Subject: [PATCH 049/102] Missed a getEnvironmentForHost() call --- test/views/github-dotcom-markdown.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/views/github-dotcom-markdown.test.js b/test/views/github-dotcom-markdown.test.js index a72a114c31..6ee2c06f79 100644 --- a/test/views/github-dotcom-markdown.test.js +++ b/test/views/github-dotcom-markdown.test.js @@ -4,6 +4,7 @@ import {shell} from 'electron'; import {BareGithubDotcomMarkdown} from '../../lib/views/github-dotcom-markdown'; import {handleClickEvent, openIssueishLinkInNewTab, openLinkInBrowser} from '../../lib/views/issueish-link'; +import {getEndpoint} from '../../lib/models/endpoint'; import RelayNetworkLayerManager from '../../lib/relay-network-layer-manager'; import * as reporterProxy from '../../lib/reporter-proxy'; @@ -11,7 +12,8 @@ describe('GithubDotcomMarkdown', function() { let relayEnvironment; beforeEach(function() { - relayEnvironment = RelayNetworkLayerManager.getEnvironmentForHost('https://api.somehost.com', '1234'); + const endpoint = getEndpoint('somehost.com'); + relayEnvironment = RelayNetworkLayerManager.getEnvironmentForHost(endpoint, '1234'); }); function buildApp(overloadProps = {}) { From 439de1feafb39d9a03764390d797c6109f566c68 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 15:19:28 -0500 Subject: [PATCH 050/102] Spelling --- lib/models/enableable-operation.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/enableable-operation.js b/lib/models/enableable-operation.js index bd651cb5e3..4a69c8dce8 100644 --- a/lib/models/enableable-operation.js +++ b/lib/models/enableable-operation.js @@ -3,7 +3,7 @@ const ENABLED = Symbol('enabled'); const NO_REASON = Symbol('no-reason'); // Track an operation that may be either enabled or disabled with a message and a reason. EnableableOperation instances -// are immutable to aid passing them as React comopnent props; call `.enable()` or `.disable()` to derive a new +// are immutable to aid passing them as React component props; call `.enable()` or `.disable()` to derive a new // operation instance with the same callback. export default class EnableableOperation { constructor(op, options = {}) { From 15f6cfa3f640a030405b9b445a23ac25a45ac841 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Thu, 13 Dec 2018 15:19:44 -0500 Subject: [PATCH 051/102] Use URL as the Map key --- lib/relay-network-layer-manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 5bb618dc1f..fd6abf2861 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -134,14 +134,14 @@ export default class RelayNetworkLayerManager { static getEnvironmentForHost(endpoint, token) { const url = endpoint.getGraphQLRoot(); let {environment, network} = relayEnvironmentPerURL.get(url) || {}; - tokenPerURL.set(endpoint, token); + tokenPerURL.set(url, token); if (!environment) { const source = new RecordSource(); const store = new Store(source); network = Network.create(this.getFetchQuery(endpoint, token)); environment = new Environment({network, store}); - relayEnvironmentPerURL.set(endpoint, {environment, network}); + relayEnvironmentPerURL.set(url, {environment, network}); } return environment; } From 30d230ca8cd52daeb8f0ebdd3eb48ffa5dd83df1 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 21:41:06 +0100 Subject: [PATCH 052/102] A++ error messages --- lib/containers/pr-changed-files-container.js | 26 +++++++++++--------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index ed8b2da8c6..3c4829b2c6 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -63,26 +63,30 @@ export default class PullRequestChangedFilesContainer extends React.Component { } async fetchDiff() { + const diffError = message => new Promise(resolve => + this.setState({isLoading: false, error: message}, resolve)); const url = this.getDiffURL(); + + const response = await fetch(url, { + headers: { + Accept: 'application/vnd.github.v3.diff', + Authorization: `bearer ${this.props.token}`, + }, + // eslint-disable-next-line handle-callback-err + }).catch(err => { + diffError(`Network error encountered at fetching ${url}`); + }); + try { - const response = await fetch(url, { - headers: { - Accept: 'application/vnd.github.v3.diff', - Authorization: `bearer ${this.props.token}`, - }, - }); if (response && response.ok) { const rawDiff = await response.text(); const multiFilePatch = this.buildPatch(rawDiff); await new Promise(resolve => this.setState({isLoading: false, multiFilePatch}, resolve)); } else { - throw new Error(response.statusText); + diffError(`Unable to fetch diff${response ? ' : ' + response.statusText : ''}.`); } } catch (e) { - // eslint-disable-next-line no-console - console.error(e); - await new Promise(resolve => - this.setState({isLoading: false, error: 'Unable to load diff for this PR.'}, resolve)); + diffError('Unable to parse diff.'); } } From 870c1780f6deeb5a182006a4d4af49608324e1b8 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 21:41:16 +0100 Subject: [PATCH 053/102] reset error state upon refresh --- lib/containers/pr-changed-files-container.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 3c4829b2c6..c89f14872b 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -40,13 +40,13 @@ export default class PullRequestChangedFilesContainer extends React.Component { constructor(props) { super(props); - this.state = {isLoading: true}; + this.state = {isLoading: true, error: null}; this.fetchDiff(); } componentDidUpdate(prevProps) { if (this.props.shouldRefetch && !prevProps.shouldRefetch) { - this.setState({isLoading: true}); + this.setState({isLoading: true, error: null}); this.fetchDiff(); } } From 8472a77dc6b49ce66f0227ab4b2274a9c6c3c4ef Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 21:52:48 +0100 Subject: [PATCH 054/102] copy! --- lib/containers/pr-changed-files-container.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index c89f14872b..d93b2285ca 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -83,10 +83,10 @@ export default class PullRequestChangedFilesContainer extends React.Component { const multiFilePatch = this.buildPatch(rawDiff); await new Promise(resolve => this.setState({isLoading: false, multiFilePatch}, resolve)); } else { - diffError(`Unable to fetch diff${response ? ' : ' + response.statusText : ''}.`); + diffError(`Unable to fetch diff for this pull request${response ? ' : ' + response.statusText : ''}.`); } } catch (e) { - diffError('Unable to parse diff.'); + diffError('Unable to parse diff for this pull request.'); } } From 446741e48f8f8c9f283df19d0381f047547a8cc3 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 21:56:43 +0100 Subject: [PATCH 055/102] leverage the existing ErrorView --- lib/containers/pr-changed-files-container.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index d93b2285ca..95cccfbb9d 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -5,6 +5,7 @@ import {parse as parseDiff} from 'what-the-diff'; import {ItemTypePropType, EndpointPropType} from '../prop-types'; import MultiFilePatchController from '../controllers/multi-file-patch-controller'; import LoadingView from '../views/loading-view'; +import ErrorView from '../views/error-view'; import {buildMultiFilePatch} from '../models/patch'; export default class PullRequestChangedFilesContainer extends React.Component { @@ -96,7 +97,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { } if (this.state.error) { - return
{this.state.error}
; + return ; } return ( From 624da7a4f8ca020cde6afaee59f0dd76c2fb703f Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 21:56:55 +0100 Subject: [PATCH 056/102] don't always show logout button on errorview --- lib/views/error-view.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/views/error-view.js b/lib/views/error-view.js index a8d8067997..21e47d2200 100644 --- a/lib/views/error-view.js +++ b/lib/views/error-view.js @@ -35,7 +35,9 @@ export default class ErrorView extends React.Component { {this.props.retry && ( )} - + {this.props.logout && ( + + )} From 52623a179dcfe1be6766c5e1c9e09ba9fbdf90a1 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 13:02:52 -0800 Subject: [PATCH 057/102] add test for when fetch fails --- .../pr-changed-files-container.test.js | 111 ++++++++++-------- 1 file changed, 65 insertions(+), 46 deletions(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index 6b0a24c3ea..e43ebcb225 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -21,6 +21,14 @@ describe('PullRequestChangedFilesContainer', function() { token="1234" endpoint={getEndpoint('github.com')} itemType={IssueishDetailItem} + destroy={() => {}} + shouldRefetch={false} + workspace={{}} + commands={{}} + keymaps={{}} + tooltips={{}} + config={{}} + localRepository={{}} {...overrideProps} /> ); @@ -33,58 +41,69 @@ describe('PullRequestChangedFilesContainer', function() { }); } - beforeEach(function() { - setDiffResponse(rawDiff); - sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); - }); - - it('renders a loading spinner if data has not yet been fetched', function() { - const wrapper = shallow(buildApp()); - assert.isTrue(wrapper.find('LoadingView').exists()); - }); - - it('passes extra props through to PullRequestChangedFilesController', async function() { - const extraProp = Symbol('really really extra'); - - const wrapper = shallow(buildApp({extraProp})); - await assert.async.isTrue(wrapper.update().find('MultiFilePatchController').exists()); - - const controller = wrapper.find('MultiFilePatchController'); - assert.strictEqual(controller.prop('extraProp'), extraProp); - }); + describe('when the data is able to be fetched successfully', function() { + beforeEach(function() { + setDiffResponse(rawDiff); + sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); + }); + it('renders a loading spinner if data has not yet been fetched', function() { + const wrapper = shallow(buildApp()); + assert.isTrue(wrapper.find('LoadingView').exists()); + }); + it('passes extra props through to PullRequestChangedFilesController', async function() { + const extraProp = Symbol('really really extra'); - it('builds the diff URL', function() { - const wrapper = shallow(buildApp({ - owner: 'smashwilson', - repo: 'pushbot', - number: 12, - endpoint: getEndpoint('github.com'), - })); + const wrapper = shallow(buildApp({extraProp})); + await assert.async.isTrue(wrapper.update().find('MultiFilePatchController').exists()); - const diffURL = wrapper.instance().getDiffURL(); - assert.strictEqual(diffURL, 'https://api.github.com/repos/smashwilson/pushbot/pulls/12'); - }); + const controller = wrapper.find('MultiFilePatchController'); + assert.strictEqual(controller.prop('extraProp'), extraProp); + }); - it('passes loaded diff data through to the controller', async function() { - const wrapper = shallow(buildApp({ - token: '4321', - })); - await assert.async.isTrue(wrapper.update().find('MultiFilePatchController').exists()); + it('builds the diff URL', function() { + const wrapper = shallow(buildApp({ + owner: 'smashwilson', + repo: 'pushbot', + number: 12, + endpoint: getEndpoint('github.com'), + })); - const controller = wrapper.find('MultiFilePatchController'); - const expected = buildMultiFilePatch(parseDiff(rawDiff)); - assert.isTrue(controller.prop('multiFilePatch').isEqual(expected)); + const diffURL = wrapper.instance().getDiffURL(); + assert.strictEqual(diffURL, 'https://api.github.com/repos/smashwilson/pushbot/pulls/12'); + }); - assert.deepEqual(window.fetch.lastCall.args, [ - 'https://api.github.com/repos/atom/github/pulls/1804', - { - headers: { - Accept: 'application/vnd.github.v3.diff', - Authorization: 'bearer 4321', + it('passes loaded diff data through to the controller', async function() { + const wrapper = shallow(buildApp({ + token: '4321', + })); + await assert.async.isTrue(wrapper.update().find('MultiFilePatchController').exists()); + + const controller = wrapper.find('MultiFilePatchController'); + const expected = buildMultiFilePatch(parseDiff(rawDiff)); + assert.isTrue(controller.prop('multiFilePatch').isEqual(expected)); + + assert.deepEqual(window.fetch.lastCall.args, [ + 'https://api.github.com/repos/atom/github/pulls/1804', + { + headers: { + Accept: 'application/vnd.github.v3.diff', + Authorization: 'bearer 4321', + }, }, - }, - ]); + ]); + }); }); - it('renders an error if fetch returns a non-ok response'); + describe('when fetch fails', function() { + it('renders an error if fetch returns a non-ok response', async function() { + const badResponse = new window.Response(rawDiff, { + status: 404, + statusText: 'oh noes', + headers: {'Content-type': 'text/plain'}, + }); + sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(badResponse)); + const wrapper = shallow(buildApp()); + await assert.async.strictEqual(wrapper.update().instance().state.error, 'Unable to load diff for this PR.'); + }); + }); }); From 7f8707d9e33c50350cf0e2873154368bc9650e84 Mon Sep 17 00:00:00 2001 From: Vanessa Yuen Date: Thu, 13 Dec 2018 22:17:19 +0100 Subject: [PATCH 058/102] update component atlas --- docs/react-component-atlas.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/react-component-atlas.md b/docs/react-component-atlas.md index a619d910d8..fe8264a752 100644 --- a/docs/react-component-atlas.md +++ b/docs/react-component-atlas.md @@ -118,6 +118,13 @@ This is a high-level overview of the structure of the React component tree that > > > [``](/lib/views/pr-commit-view.js) > > > > > > Enumerate the commits associated with a pull request. +> > +> > > [``](/lib/containers/pr-changed-files-container.js) +> > > +> > > Show all the changes, separated by files, introduced in a pull request. +> > > +> > > > [``](/lib/controllers/multi-file-patch-controller.js) +> > > > [``](/lib/views/multi-file-patch-view.js) > > > [``](/lib/views/init-dialog.js) > > [``](/lib/views/clone-dialog.js) From 18a89737fa2e811b197bad073d1c4438e65e5262 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 13:36:39 -0800 Subject: [PATCH 059/102] fix test to deal with improved error handling --- lib/containers/pr-changed-files-container.js | 2 +- test/containers/pr-changed-files-container.test.js | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index 95cccfbb9d..09e65a360f 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -84,7 +84,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { const multiFilePatch = this.buildPatch(rawDiff); await new Promise(resolve => this.setState({isLoading: false, multiFilePatch}, resolve)); } else { - diffError(`Unable to fetch diff for this pull request${response ? ' : ' + response.statusText : ''}.`); + diffError(`Unable to fetch diff for this pull request${response ? ': ' + response.statusText : ''}.`); } } catch (e) { diffError('Unable to parse diff for this pull request.'); diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index e43ebcb225..e6d002971f 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -94,7 +94,7 @@ describe('PullRequestChangedFilesContainer', function() { }); }); - describe('when fetch fails', function() { + describe.only('when fetch fails', function() { it('renders an error if fetch returns a non-ok response', async function() { const badResponse = new window.Response(rawDiff, { status: 404, @@ -103,7 +103,10 @@ describe('PullRequestChangedFilesContainer', function() { }); sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(badResponse)); const wrapper = shallow(buildApp()); - await assert.async.strictEqual(wrapper.update().instance().state.error, 'Unable to load diff for this PR.'); + const expectedErrorMessage = 'Unable to fetch diff for this pull request: oh noes.'; + await assert.async.deepEqual(wrapper.update().instance().state.error, expectedErrorMessage); + const errorView = wrapper.find('ErrorView'); + assert.deepEqual(errorView.prop('descriptions'), [expectedErrorMessage]); }); }); }); From 66e5a62cb41bb3dee097ca480e67d1c8b0b55f49 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 13:41:17 -0800 Subject: [PATCH 060/102] :shirt: --- .../pr-changed-files-container.test.js | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index e6d002971f..d45c2aa4e7 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -94,19 +94,19 @@ describe('PullRequestChangedFilesContainer', function() { }); }); - describe.only('when fetch fails', function() { + describe('when fetch fails', function() { it('renders an error if fetch returns a non-ok response', async function() { - const badResponse = new window.Response(rawDiff, { - status: 404, - statusText: 'oh noes', - headers: {'Content-type': 'text/plain'}, - }); - sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(badResponse)); - const wrapper = shallow(buildApp()); - const expectedErrorMessage = 'Unable to fetch diff for this pull request: oh noes.'; - await assert.async.deepEqual(wrapper.update().instance().state.error, expectedErrorMessage); - const errorView = wrapper.find('ErrorView'); - assert.deepEqual(errorView.prop('descriptions'), [expectedErrorMessage]); + const badResponse = new window.Response(rawDiff, { + status: 404, + statusText: 'oh noes', + headers: {'Content-type': 'text/plain'}, }); + sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(badResponse)); + const wrapper = shallow(buildApp()); + const expectedErrorMessage = 'Unable to fetch diff for this pull request: oh noes.'; + await assert.async.deepEqual(wrapper.update().instance().state.error, expectedErrorMessage); + const errorView = wrapper.find('ErrorView'); + assert.deepEqual(errorView.prop('descriptions'), [expectedErrorMessage]); + }); }); }); From aa5f337e75f72c07d2b4adf1e401904ea7ff126d Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 14:00:07 -0800 Subject: [PATCH 061/102] test diff parsing error state --- .../pr-changed-files-container.test.js | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index d45c2aa4e7..005baed4f9 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -94,7 +94,20 @@ describe('PullRequestChangedFilesContainer', function() { }); }); - describe('when fetch fails', function() { + describe('error states', function() { + async function assertErrorRendered(expectedErrorMessage, wrapper) { + await assert.async.deepEqual(wrapper.update().instance().state.error, expectedErrorMessage); + const errorView = wrapper.find('ErrorView'); + assert.deepEqual(errorView.prop('descriptions'), [expectedErrorMessage]); + } + it('renders an error if diff parsing fails', async function() { + setDiffResponse('bad diff no treat for you'); + sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); + const wrapper = shallow(buildApp()); + const expectedErrorMessage = 'Unable to parse diff for this pull request.'; + await assertErrorRendered('Unable to parse diff for this pull request.', wrapper); + }); + it('renders an error if fetch returns a non-ok response', async function() { const badResponse = new window.Response(rawDiff, { status: 404, @@ -103,10 +116,7 @@ describe('PullRequestChangedFilesContainer', function() { }); sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(badResponse)); const wrapper = shallow(buildApp()); - const expectedErrorMessage = 'Unable to fetch diff for this pull request: oh noes.'; - await assert.async.deepEqual(wrapper.update().instance().state.error, expectedErrorMessage); - const errorView = wrapper.find('ErrorView'); - assert.deepEqual(errorView.prop('descriptions'), [expectedErrorMessage]); + await assertErrorRendered('Unable to fetch diff for this pull request: oh noes.', wrapper); }); }); }); From 424b5c41b9ba71b11829342abca7953ad4903eca Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 14:40:13 -0800 Subject: [PATCH 062/102] :shirt: againnn --- test/containers/pr-changed-files-container.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index 005baed4f9..78a98d1a71 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -104,7 +104,6 @@ describe('PullRequestChangedFilesContainer', function() { setDiffResponse('bad diff no treat for you'); sinon.stub(window, 'fetch').callsFake(() => Promise.resolve(diffResponse)); const wrapper = shallow(buildApp()); - const expectedErrorMessage = 'Unable to parse diff for this pull request.'; await assertErrorRendered('Unable to parse diff for this pull request.', wrapper); }); From 879c6389a4fbe461faf8c225c9a4c3388e8bdd4f Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 14:47:27 -0800 Subject: [PATCH 063/102] istanbul ignore dev debugging code to log rate limits --- lib/relay-network-layer-manager.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index fd6abf2861..f378fc9624 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -105,6 +105,7 @@ function createFetchQuery(url) { }); try { + /* istanbul ignore next */ atom && atom.inDevMode() && logRatelimitApi(response.headers); } catch (_e) { /* do nothing */ } From aeae3b35e2a4b92667d5842ceb2cf8da054a81f1 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 16:03:10 -0800 Subject: [PATCH 064/102] add test for refetching data --- test/containers/pr-changed-files-container.test.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/containers/pr-changed-files-container.test.js b/test/containers/pr-changed-files-container.test.js index 78a98d1a71..c6f972dce7 100644 --- a/test/containers/pr-changed-files-container.test.js +++ b/test/containers/pr-changed-files-container.test.js @@ -92,7 +92,15 @@ describe('PullRequestChangedFilesContainer', function() { }, ]); }); - }); + it('re fetches data when shouldRefetch is true', async function() { + const wrapper = shallow(buildApp()); + await assert.async.isTrue(wrapper.update().find('MultiFilePatchController').exists()); + assert.strictEqual(window.fetch.callCount, 1); + wrapper.setProps({shouldRefetch: true}); + assert.isTrue(wrapper.instance().state.isLoading); + await assert.async.strictEqual(window.fetch.callCount, 2); + }); + }); describe('error states', function() { async function assertErrorRendered(expectedErrorMessage, wrapper) { From 714ec4b2f3696a4369766b138279f8b55e988044 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Thu, 13 Dec 2018 16:10:30 -0800 Subject: [PATCH 065/102] more istanbul ignoring --- lib/prop-types.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/prop-types.js b/lib/prop-types.js index bb3a298687..78fd797320 100644 --- a/lib/prop-types.js +++ b/lib/prop-types.js @@ -180,6 +180,7 @@ function createItemTypePropType(required) { } if (props[propName] === undefined || props[propName] === null) { + /* istanbul ignore else */ if (required) { return new Error(`Missing required prop ${propName} on component ${componentName}.`); } else { @@ -187,6 +188,7 @@ function createItemTypePropType(required) { } } + /* istanbul ignore if */ if (!lazyItemConstructors.has(props[propName])) { const choices = Array.from(lazyItemConstructors, each => each.name).join(', '); return new Error( From d5a0f630ceb8907110c934f7a4d48aa4ac10e07c Mon Sep 17 00:00:00 2001 From: simurai Date: Fri, 14 Dec 2018 11:41:19 +0900 Subject: [PATCH 066/102] Fix PrTimeline scrolling issues --- styles/pr-timeline.less | 1 + 1 file changed, 1 insertion(+) diff --git a/styles/pr-timeline.less b/styles/pr-timeline.less index 99c4e8f9f0..8a636612a5 100644 --- a/styles/pr-timeline.less +++ b/styles/pr-timeline.less @@ -3,6 +3,7 @@ @avatar-size: 20px; .github-PrTimeline { + position: relative; margin-top: @component-padding * 3; border-top: 1px solid mix(@text-color, @base-background-color, 15%); From d84707e0e41e0de254ed3994c381da74b7f8ca10 Mon Sep 17 00:00:00 2001 From: simurai Date: Fri, 14 Dec 2018 19:57:30 +0900 Subject: [PATCH 067/102] Make header more compact --- lib/views/pr-detail-view.js | 97 +++++++++++++++--------------- styles/github-dotcom-markdown.less | 7 +++ styles/issueish-detail-view.less | 91 ++++++++++++++-------------- styles/pr-commit-view.less | 4 +- styles/pr-statuses.less | 1 - styles/pr-timeline.less | 19 +++--- 6 files changed, 111 insertions(+), 108 deletions(-) diff --git a/lib/views/pr-detail-view.js b/lib/views/pr-detail-view.js index bd9d9a8083..62dc2510c9 100644 --- a/lib/views/pr-detail-view.js +++ b/lib/views/pr-detail-view.js @@ -120,21 +120,20 @@ export class BarePullRequestDetailView extends React.Component { } renderPrMetadata(pullRequest, repo) { + // TODO: Move these infos to the tabs. E.g. Commits (3) + // {pullRequest.author.login} wants to merge{' '} + // {pullRequest.countedCommits.totalCount} commits and{' '} + // {pullRequest.changedFiles} changed files into{' '} return ( -
- - {pullRequest.author.login} wants to merge{' '} - {pullRequest.countedCommits.totalCount} commits and{' '} - {pullRequest.changedFiles} changed files into{' '} - {pullRequest.isCrossRepository ? - `${repo.owner.login}/${pullRequest.baseRefName}` : pullRequest.baseRefName} from{' '} - {pullRequest.isCrossRepository ? - `${pullRequest.author.login}/${pullRequest.headRefName}` : pullRequest.headRefName} - -
+ + {pullRequest.isCrossRepository ? + `${repo.owner.login}/${pullRequest.baseRefName}` : pullRequest.baseRefName}{' ‹ '} + {pullRequest.isCrossRepository ? + `${pullRequest.author.login}/${pullRequest.headRefName}` : pullRequest.headRefName} + ); } @@ -167,18 +166,19 @@ export class BarePullRequestDetailView extends React.Component { {/* overview */} - No description provided.'} - switchToIssueish={this.props.switchToIssueish} - /> - - - +
+ No description provided.'} + switchToIssueish={this.props.switchToIssueish} + /> + + +
{/* build status */} @@ -230,32 +230,30 @@ export class BarePullRequestDetailView extends React.Component {
-
+ {this.renderPullRequestBody(pullRequest)}
-
+
@@ -256,6 +256,8 @@ export class BarePullRequestDetailView extends React.Component { +
+
{this.renderPrMetadata(pullRequest, repo)}
diff --git a/styles/issueish-detail-view.less b/styles/issueish-detail-view.less index e33131af99..b6ed2f685c 100644 --- a/styles/issueish-detail-view.less +++ b/styles/issueish-detail-view.less @@ -41,12 +41,18 @@ &-headerColumn { &.is-flexible { flex: 1; + display: flex; + flex-wrap: wrap; } } &-headerRow { display: flex; align-items: center; + margin: 2px 0; + &.is-fullwidth { + flex: 1 1 100%; + } } // Avatar ------------------------ @@ -57,7 +63,7 @@ } &-title { - margin: 0 0 .25em 0; + margin: 0; font-size: 1.25em; font-weight: 500; line-height: 1.3; From 3d5018aa2762f92dbe2dc462ba92874cbbc3bc32 Mon Sep 17 00:00:00 2001 From: simurai Date: Mon, 17 Dec 2018 16:51:22 +0900 Subject: [PATCH 083/102] Make headerLink wrap --- styles/issueish-detail-view.less | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/styles/issueish-detail-view.less b/styles/issueish-detail-view.less index b6ed2f685c..1f35fb3d2e 100644 --- a/styles/issueish-detail-view.less +++ b/styles/issueish-detail-view.less @@ -49,6 +49,7 @@ &-headerRow { display: flex; align-items: center; + flex-wrap: wrap; margin: 2px 0; &.is-fullwidth { flex: 1 1 100%; @@ -81,7 +82,6 @@ } &-headerLink { - margin-left: @component-padding; color: @text-color-subtle; } @@ -99,6 +99,7 @@ &-headerRefreshButton { display: inline-block; + margin-right: @component-padding; color: @text-color-subtle; cursor: pointer; From c62bb517e1d9297bd6734395656393e0e01f5809 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 17 Dec 2018 08:22:20 -0800 Subject: [PATCH 084/102] for realsies istanbul ignore relay-network-layer-manager --- .nycrc.json | 3 ++- lib/relay-network-layer-manager.js | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.nycrc.json b/.nycrc.json index b9380f0e5c..7135cec850 100644 --- a/.nycrc.json +++ b/.nycrc.json @@ -6,6 +6,7 @@ ], "exclude": [ "lib/views/git-cache-view.js", - "lib/views/git-timings-view.js" + "lib/views/git-timings-view.js", + "lib/relay-network-layer-manager.js" ] } diff --git a/lib/relay-network-layer-manager.js b/lib/relay-network-layer-manager.js index 555d86b931..fd6abf2861 100644 --- a/lib/relay-network-layer-manager.js +++ b/lib/relay-network-layer-manager.js @@ -1,4 +1,3 @@ -/* istanbul ignore next */ import util from 'util'; import {Environment, Network, RecordSource, Store} from 'relay-runtime'; import moment from 'moment'; From 14a2d270c8cdec24e0b065ea3df0ee1472a8b1f8 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 17 Dec 2018 12:20:39 -0500 Subject: [PATCH 085/102] IssueishDetailItem uses `github.com` as its host --- lib/controllers/root-controller.js | 2 +- test/controllers/root-controller.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/controllers/root-controller.js b/lib/controllers/root-controller.js index 2205e08611..b911f11b3e 100644 --- a/lib/controllers/root-controller.js +++ b/lib/controllers/root-controller.js @@ -588,7 +588,7 @@ export default class RootController extends React.Component { } acceptOpenIssueish({repoOwner, repoName, issueishNumber}) { - const uri = IssueishDetailItem.buildURI('https://api.github.com', repoOwner, repoName, issueishNumber); + const uri = IssueishDetailItem.buildURI('github.com', repoOwner, repoName, issueishNumber); this.setState({openIssueishDialogActive: false}); this.props.workspace.open(uri).then(() => { addEvent('open-issueish-in-pane', {package: 'github', from: 'dialog'}); diff --git a/test/controllers/root-controller.test.js b/test/controllers/root-controller.test.js index 5c016c7dd3..e3bf7c6530 100644 --- a/test/controllers/root-controller.test.js +++ b/test/controllers/root-controller.test.js @@ -428,7 +428,7 @@ describe('RootController', function() { resolveOpenIssueish(); await promise; - const uri = IssueishDetailItem.buildURI('https://api.github.com', repoOwner, repoName, issueishNumber); + const uri = IssueishDetailItem.buildURI('github.com', repoOwner, repoName, issueishNumber); assert.isTrue(openIssueishDetails.calledWith(uri)); From 3397d530bb778c9b98989be3e8e1f62845f83125 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 17 Dec 2018 12:23:55 -0500 Subject: [PATCH 086/102] workdirPath is missing when looking at an issueish from another repo --- lib/controllers/issueish-detail-controller.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/controllers/issueish-detail-controller.js b/lib/controllers/issueish-detail-controller.js index 360189ef3b..f1991ab5e3 100644 --- a/lib/controllers/issueish-detail-controller.js +++ b/lib/controllers/issueish-detail-controller.js @@ -32,6 +32,7 @@ export class BareIssueishDetailController extends React.Component { isAbsent: PropTypes.bool.isRequired, isLoading: PropTypes.bool.isRequired, isPresent: PropTypes.bool.isRequired, + workdirPath: PropTypes.string, // Connection information endpoint: EndpointPropType.isRequired, @@ -39,7 +40,6 @@ export class BareIssueishDetailController extends React.Component { // Atom environment workspace: PropTypes.object.isRequired, - workdirPath: PropTypes.string.isRequired, commands: PropTypes.object.isRequired, keymaps: PropTypes.object.isRequired, tooltips: PropTypes.object.isRequired, @@ -277,6 +277,11 @@ export class BareIssueishDetailController extends React.Component { } openCommit = async ({sha}) => { + /* istanbul ignore if */ + if (!this.props.workdirPath) { + return; + } + const uri = CommitDetailItem.buildURI(this.props.workdirPath, sha); await this.props.workspace.open(uri, {pending: true}); addEvent('open-commit-in-pane', {package: 'github', from: this.constructor.name}); From 788dd705b8f2b108b48ce96d3036b76f83552643 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 17 Dec 2018 12:26:15 -0500 Subject: [PATCH 087/102] onBranch and openCommit will not be set for issues --- lib/views/issueish-timeline-view.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/views/issueish-timeline-view.js b/lib/views/issueish-timeline-view.js index 6523173807..923f919abc 100644 --- a/lib/views/issueish-timeline-view.js +++ b/lib/views/issueish-timeline-view.js @@ -77,8 +77,13 @@ export default class IssueishTimelineView extends React.Component { pullRequest: PropTypes.shape({ timeline: TimelineConnectionPropType, }), - onBranch: PropTypes.bool.isRequired, - openCommit: PropTypes.func.isRequired, + onBranch: PropTypes.bool, + openCommit: PropTypes.func, + } + + static defaultProps = { + onBranch: false, + openCommit: () => {}, } constructor(props) { From e2bfeea6c08b95a0d18a85ed90a12f5a40edb63e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 17 Dec 2018 13:51:07 -0500 Subject: [PATCH 088/102] Pass Endpoint into RemoteContainer separately --- lib/containers/remote-container.js | 25 ++++++++++++------------ lib/views/github-tab-view.js | 7 +++++-- test/containers/remote-container.test.js | 1 - 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/containers/remote-container.js b/lib/containers/remote-container.js index 1ef7a37033..e4aec30e9a 100644 --- a/lib/containers/remote-container.js +++ b/lib/containers/remote-container.js @@ -4,7 +4,9 @@ import {QueryRenderer, graphql} from 'react-relay'; import {incrementCounter} from '../reporter-proxy'; import {autobind} from '../helpers'; -import {RemotePropType, RemoteSetPropType, BranchSetPropType, OperationStateObserverPropType} from '../prop-types'; +import { + RemotePropType, RemoteSetPropType, BranchSetPropType, OperationStateObserverPropType, EndpointPropType, +} from '../prop-types'; import RelayNetworkLayerManager from '../relay-network-layer-manager'; import {UNAUTHENTICATED, INSUFFICIENT} from '../shared/keytar-strategy'; import RemoteController from '../controllers/remote-controller'; @@ -15,18 +17,21 @@ import GithubLoginView from '../views/github-login-view'; export default class RemoteContainer extends React.Component { static propTypes = { + // Connection loginModel: PropTypes.object.isRequired, + endpoint: EndpointPropType.isRequired, + // Repository attributes remoteOperationObserver: OperationStateObserverPropType.isRequired, + pushInProgress: PropTypes.bool.isRequired, workingDirectory: PropTypes.string.isRequired, workspace: PropTypes.object.isRequired, remote: RemotePropType.isRequired, remotes: RemoteSetPropType.isRequired, branches: BranchSetPropType.isRequired, - aheadCount: PropTypes.number, - pushInProgress: PropTypes.bool.isRequired, + // Action methods onPushBranch: PropTypes.func.isRequired, } @@ -37,7 +42,7 @@ export default class RemoteContainer extends React.Component { } fetchToken(loginModel) { - return loginModel.getToken(this.endpoint().getLoginAccount()); + return loginModel.getToken(this.props.endpoint.getLoginAccount()); } render() { @@ -67,7 +72,7 @@ export default class RemoteContainer extends React.Component { ); } - const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.endpoint(), token); + const environment = RelayNetworkLayerManager.getEnvironmentForHost(this.props.endpoint, token); const query = graphql` query remoteContainerQuery($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { @@ -112,7 +117,7 @@ export default class RemoteContainer extends React.Component { return ( this.props.handlePushBranch(this.props.currentBranch, this.props.currentRemote)} remote={this.props.currentRemote} remotes={this.props.remotes} branches={this.props.branches} aheadCount={this.props.aheadCount} - pushInProgress={this.props.pushInProgress} + + onPushBranch={() => this.props.handlePushBranch(this.props.currentBranch, this.props.currentRemote)} /> ); } diff --git a/test/containers/remote-container.test.js b/test/containers/remote-container.test.js index 87c651a0c2..2d60943684 100644 --- a/test/containers/remote-container.test.js +++ b/test/containers/remote-container.test.js @@ -34,7 +34,6 @@ describe('RemoteContainer', function() { return ( Date: Mon, 17 Dec 2018 10:56:59 -0800 Subject: [PATCH 089/102] add tests for UserStore uncovered lines --- lib/models/user-store.js | 21 ++++++++------ test/models/user-store.test.js | 50 +++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/lib/models/user-store.js b/lib/models/user-store.js index 1f27ac7c7e..ae11ba48a2 100644 --- a/lib/models/user-store.js +++ b/lib/models/user-store.js @@ -113,6 +113,17 @@ export default class UserStore { ); } + async getToken(loginModel, loginAccount) { + if (!loginModel) { + return null; + } + const token = await loginModel.getToken(loginAccount); + if (token === UNAUTHENTICATED || token === INSUFFICIENT) { + return null; + } + return token; + } + async loadMentionableUsers(remote) { const cached = this.cache.get(remote); if (cached !== null) { @@ -121,14 +132,8 @@ export default class UserStore { } const endpoint = remote.getEndpoint(); - - const loginModel = this.loginObserver.getActiveModel(); - if (!loginModel) { - return; - } - - const token = await loginModel.getToken(endpoint.getLoginAccount()); - if (token === UNAUTHENTICATED || token === INSUFFICIENT) { + const token = await this.getToken(this.loginObserver.getActiveModel(), endpoint.getLoginAccount()); + if (!token) { return; } diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 10ef5c993c..61f46a4694 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -3,7 +3,7 @@ import dedent from 'dedent-js'; import UserStore, {source} from '../../lib/models/user-store'; import Author, {nullAuthor} from '../../lib/models/author'; import GithubLoginModel from '../../lib/models/github-login-model'; -import {InMemoryStrategy} from '../../lib/shared/keytar-strategy'; +import {InMemoryStrategy, UNAUTHENTICATED} from '../../lib/shared/keytar-strategy'; import {expectRelayQuery} from '../../lib/relay-network-layer-manager'; import {cloneRepository, buildRepository, FAKE_USER} from '../helpers'; @@ -365,6 +365,51 @@ describe('UserStore', function() { ]); }); + describe.only('getToken', function() { + let repository, workdirPath; + beforeEach(async function() { + workdirPath = await cloneRepository('multiple-commits'); + repository = await buildRepository(workdirPath); + }) + it('returns null if loginModel is falsy', async function() { + store = new UserStore({repository, login, config}); + const token = await store.getToken(undefined, 'https://api.github.com'); + assert.isNull(token); + }); + + it('returns null if token is INSUFFICIENT', async function() { + const loginModel = new GithubLoginModel(InMemoryStrategy); + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org'])); + + await loginModel.setToken('https://api.github.com', '1234'); + store = new UserStore({repository, loginModel, config}); + const token = await store.getToken(loginModel, 'https://api.github.com'); + assert.isNull(token); + }); + + it('returns null if token is UNAUTHENTICATED', async function() { + const loginModel = new GithubLoginModel(InMemoryStrategy); + sinon.stub(loginModel, 'getToken').returns(Promise.resolve(UNAUTHENTICATED)); + + store = new UserStore({repository, loginModel, config}); + const getToken = await store.getToken(loginModel, 'https://api.github.com'); + assert.isNull(getToken); + }); + + it('return token if token is sufficient and model is truthy', async function() { + const loginModel = new GithubLoginModel(InMemoryStrategy); + sinon.stub(loginModel, 'getScopes').returns(Promise.resolve(['repo', 'read:org', 'user:email'])); + + const expectedToken = '1234'; + await loginModel.setToken('https://api.github.com', expectedToken); + store = new UserStore({repository, loginModel, config}); + const actualToken = await store.getToken(loginModel, 'https://api.github.com'); + assert.strictEqual(expectedToken, actualToken); + }); + + }); + + describe('GraphQL response caching', function() { it('caches mentionable users acquired from GraphQL', async function() { await login.setToken('https://api.github.com', '1234'); @@ -384,6 +429,7 @@ describe('UserStore', function() { store = new UserStore({repository, login, config}); sinon.spy(store, 'loadUsers'); + sinon.spy(store, 'getToken'); // The first update is triggered by the commiter, the second from GraphQL results arriving. await nextUpdatePromise(); @@ -396,6 +442,8 @@ describe('UserStore', function() { await assert.async.strictEqual(store.loadUsers.callCount, 2); await store.loadUsers.returnValues[1]; + await assert.async.strictEqual(store.getToken.callCount, 1) + assert.deepEqual(store.getUsers(), [ new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), new Author('mona@lisa.com', 'Mona Lisa', 'octocat'), From 83c8ecb3a4e4529a617f7f0bdcff1c4c2f60ba73 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 17 Dec 2018 11:01:14 -0800 Subject: [PATCH 090/102] stupid .only --- test/models/user-store.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 61f46a4694..deec063032 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -365,7 +365,7 @@ describe('UserStore', function() { ]); }); - describe.only('getToken', function() { + describe('getToken', function() { let repository, workdirPath; beforeEach(async function() { workdirPath = await cloneRepository('multiple-commits'); From b745c3724f1a11825a9edb809459117fc2506908 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 17 Dec 2018 11:32:39 -0800 Subject: [PATCH 091/102] :shirt: I really need to fix my in editor linting. --- test/models/user-store.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index deec063032..6e33d407d0 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -370,7 +370,7 @@ describe('UserStore', function() { beforeEach(async function() { workdirPath = await cloneRepository('multiple-commits'); repository = await buildRepository(workdirPath); - }) + }); it('returns null if loginModel is falsy', async function() { store = new UserStore({repository, login, config}); const token = await store.getToken(undefined, 'https://api.github.com'); @@ -442,7 +442,7 @@ describe('UserStore', function() { await assert.async.strictEqual(store.loadUsers.callCount, 2); await store.loadUsers.returnValues[1]; - await assert.async.strictEqual(store.getToken.callCount, 1) + await assert.async.strictEqual(store.getToken.callCount, 1); assert.deepEqual(store.getUsers(), [ new Author('smashwilson@github.com', 'Ash Wilson', 'smashwilson'), From b9b29c37f6f36c76468311f011a7790dbd37cdf6 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 17 Dec 2018 11:45:33 -0800 Subject: [PATCH 092/102] log console errors in `PullRequestChangedFilesContainer` --- lib/containers/pr-changed-files-container.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/containers/pr-changed-files-container.js b/lib/containers/pr-changed-files-container.js index e569f733fb..ece3bf4a82 100644 --- a/lib/containers/pr-changed-files-container.js +++ b/lib/containers/pr-changed-files-container.js @@ -64,8 +64,13 @@ export default class PullRequestChangedFilesContainer extends React.Component { } async fetchDiff() { - const diffError = message => new Promise(resolve => - this.setState({isLoading: false, error: message}, resolve)); + const diffError = (message, err = null) => new Promise(resolve => { + if (err) { + // eslint-disable-next-line no-console + console.error(err); + } + this.setState({isLoading: false, error: message}, resolve); + }); const url = this.getDiffURL(); const response = await fetch(url, { @@ -75,7 +80,7 @@ export default class PullRequestChangedFilesContainer extends React.Component { }, // eslint-disable-next-line handle-callback-err }).catch(err => { - diffError(`Network error encountered at fetching ${url}`); + diffError(`Network error encountered at fetching ${url}`, err); }); if (this.state.error) { return; @@ -88,8 +93,8 @@ export default class PullRequestChangedFilesContainer extends React.Component { } else { diffError(`Unable to fetch diff for this pull request${response ? ': ' + response.statusText : ''}.`); } - } catch (e) { - diffError('Unable to parse diff for this pull request.'); + } catch (err) { + diffError('Unable to parse diff for this pull request.', err); } } From 1c58579e317349414badcb11528f73e6555e4af7 Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 17 Dec 2018 13:30:00 -0800 Subject: [PATCH 093/102] add test for `UserStore.loadMentionableUsers` --- test/models/user-store.test.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/test/models/user-store.test.js b/test/models/user-store.test.js index 6e33d407d0..6d5d255928 100644 --- a/test/models/user-store.test.js +++ b/test/models/user-store.test.js @@ -408,7 +408,23 @@ describe('UserStore', function() { }); }); + describe('loadMentionableUsers', function() { + it('returns undefined if token is null', async function() { + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + await repository.setConfig('remote.origin.url', 'git@github.com:me/stuff.git'); + + store = new UserStore({repository, login, config}); + sinon.stub(store, 'getToken').returns(null); + + const remoteSet = await repository.getRemotes(); + const remote = remoteSet.byDotcomRepo.get('me/stuff')[0]; + + const users = await store.loadMentionableUsers(remote); + assert.notOk(users); + }); + }); describe('GraphQL response caching', function() { it('caches mentionable users acquired from GraphQL', async function() { @@ -431,7 +447,7 @@ describe('UserStore', function() { sinon.spy(store, 'loadUsers'); sinon.spy(store, 'getToken'); - // The first update is triggered by the commiter, the second from GraphQL results arriving. + // The first update is triggered by the committer, the second from GraphQL results arriving. await nextUpdatePromise(); await nextUpdatePromise(); From ac64240e656583d31d0d86d2388091c746b14d3e Mon Sep 17 00:00:00 2001 From: Tilde Ann Thurium Date: Mon, 17 Dec 2018 13:56:37 -0800 Subject: [PATCH 094/102] add tests for `ErrorView` --- lib/views/error-view.js | 2 +- test/views/error-view.test.js | 69 +++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 test/views/error-view.test.js diff --git a/lib/views/error-view.js b/lib/views/error-view.js index 21e47d2200..6d1228bdf6 100644 --- a/lib/views/error-view.js +++ b/lib/views/error-view.js @@ -36,7 +36,7 @@ export default class ErrorView extends React.Component { )} {this.props.logout && ( - + )}
diff --git a/test/views/error-view.test.js b/test/views/error-view.test.js new file mode 100644 index 0000000000..f36a1c93a6 --- /dev/null +++ b/test/views/error-view.test.js @@ -0,0 +1,69 @@ +import React from 'react'; +import {shallow} from 'enzyme'; + +import ErrorView from '../../lib/views/error-view'; + +describe('ErrorView', function() { + function buildApp(overrideProps = {}) { + return ( + {}} + logout={() => {}} + {...overrideProps} + /> + ); + } + it('renders title', function() { + const wrapper = shallow(buildApp()); + const title = wrapper.find('.github-Message-title'); + assert.strictEqual(title.text(), 'zomg'); + }); + + it('renders descriptions', function() { + const wrapper = shallow(buildApp()); + const descriptions = wrapper.find('.github-Message-description'); + assert.lengthOf(descriptions, 3); + assert.strictEqual(descriptions.at(0).text(), 'something'); + assert.strictEqual(descriptions.at(1).text(), 'went'); + assert.strictEqual(descriptions.at(2).text(), 'wrong'); + }); + + it('renders retry button that if retry prop is passed', function() { + const retrySpy = sinon.spy(); + const wrapper = shallow(buildApp({retry: retrySpy})); + + const retryButton = wrapper.find('.btn-primary'); + assert.strictEqual(retryButton.text(), 'Try Again'); + + assert.strictEqual(retrySpy.callCount, 0); + retryButton.simulate('click'); + assert.strictEqual(retrySpy.callCount, 1); + }); + + it('does not render retry button if retry prop is not passed', function() { + const wrapper = shallow(buildApp({retry: null})); + const retryButton = wrapper.find('.btn-primary'); + assert.lengthOf(retryButton, 0); + }); + + it('renders logout button if logout prop is passed', function() { + const logoutSpy = sinon.spy(); + const wrapper = shallow(buildApp({logout: logoutSpy})); + + const logoutButton = wrapper.find('.btn-logout'); + assert.strictEqual(logoutButton.text(), 'Logout'); + + assert.strictEqual(logoutSpy.callCount, 0); + logoutButton.simulate('click'); + assert.strictEqual(logoutSpy.callCount, 1); + }); + + it('does not render logout button if logout prop is not passed', function() { + const wrapper = shallow(buildApp({logout: null})); + const logoutButton = wrapper.find('.btn-logout'); + assert.lengthOf(logoutButton, 0); + }); +}); From 0e9e1cd98a9f14aa799bc592fdd713ee85cf8e89 Mon Sep 17 00:00:00 2001 From: simurai Date: Tue, 18 Dec 2018 10:52:26 +0900 Subject: [PATCH 095/102] Move dock styles to its own file --- styles/dock.less | 134 +++++++++++++++++++++++++++++++ styles/git-tab.less | 32 -------- styles/issueish-detail-view.less | 95 ---------------------- 3 files changed, 134 insertions(+), 127 deletions(-) create mode 100644 styles/dock.less diff --git a/styles/dock.less b/styles/dock.less new file mode 100644 index 0000000000..525c327b4e --- /dev/null +++ b/styles/dock.less @@ -0,0 +1,134 @@ +@import "variables"; + +// Dock overrides +// Here ALL the overrides when a pane/panel gets used as a dock item +// It's not too high of a priority, but still good to check (and fix) once in a while + + +// Git Panel (in bottom dock) ---------------------------------------------- + +atom-dock.bottom { + .github-Git { + flex-direction: row; + } + + .github-StagingView { + display: contents; // make these invisible to layout + } + + .github-Git > div { + flex: 1; + } + + .github-StagedChanges { + border-left: 1px solid @base-border-color; + border-right: 1px solid @base-border-color; + } + + .github-StagingView-header { + flex-wrap: wrap; + } + .github-StagingView-group:first-child .github-StagingView-header { + border-top: 1px solid @base-border-color; + } + + .github-RecentCommits { + max-height: none; + border-left: 1px solid @base-border-color; + } + +} + + +// GitHub Pane ---------------------------------------------- + +atom-dock .react-tabs__tab { + padding: none; + + &--selected { + font-weight: bold; + } +} + +atom-dock .github-IssueishDetailView { + padding: @component-padding; + font-size: @font-size; + background-color: @tool-panel-background-color; + + &-tab { + border: none; + } + + &-tablist { + border-bottom: none; + display: inline; + } + + &-header { + margin-bottom: @component-padding*1.5; + padding: 0; + border: none; + border-radius: 0; + } + + &-headerGroup { + margin: 0; + } + + &-avatarImage { + width: 24px; + height: 24px; + border-radius: @component-border-radius; + } + + &-title { + font-size: 1.25em; + font-weight: 500; + line-height: 1.3; + } + + &-reactions { + padding-top: @component-padding; + } + + &-buildStatus { + margin: @component-padding 0; + + &:empty { + display: none; + } + } + + .github-PrTimeline { + margin-top: @component-padding; + } + + .timeline-item { + margin: 0; + padding: @component-padding*1.5 0; + } + + .pre-timeline-item-icon { + position: relative; + margin: 0; + &:before { + vertical-align: middle; + } + } + + &-container > .github-DotComMarkdownHtml { + margin: @component-padding 0; + } + +} + +atom-dock .github-PrStatuses { + &-header, + &-list-item { + border-radius: 0; + padding-left: 0; + padding-right: 0; + border-left: none; + border-right: none; + } +} diff --git a/styles/git-tab.less b/styles/git-tab.less index a3bfb85e5d..54d6ff2bcf 100644 --- a/styles/git-tab.less +++ b/styles/git-tab.less @@ -45,35 +45,3 @@ } } } - -atom-dock.bottom { - .github-Git { - flex-direction: row; - } - - .github-StagingView { - display: contents; // make these invisible to layout - } - - .github-Git > div { - flex: 1; - } - - .github-StagedChanges { - border-left: 1px solid @base-border-color; - border-right: 1px solid @base-border-color; - } - - .github-StagingView-header { - flex-wrap: wrap; - } - .github-StagingView-group:first-child .github-StagingView-header { - border-top: 1px solid @base-border-color; - } - - .github-RecentCommits { - max-height: none; - border-left: 1px solid @base-border-color; - } - -} diff --git a/styles/issueish-detail-view.less b/styles/issueish-detail-view.less index 1f35fb3d2e..27c61d54b4 100644 --- a/styles/issueish-detail-view.less +++ b/styles/issueish-detail-view.less @@ -241,98 +241,3 @@ } } - - -// Dock Overrides ------------------------ -// When the IssueishDetailView is shown as a Dock item - -atom-dock .react-tabs__tab { - padding: none; - - &--selected { - font-weight: bold; - } -} - -atom-dock .github-IssueishDetailView { - padding: @component-padding; - font-size: @font-size; - background-color: @tool-panel-background-color; - - &-tab { - border: none; - } - - &-tablist { - border-bottom: none; - display: inline; - } - - &-header { - margin-bottom: @component-padding*1.5; - padding: 0; - border: none; - border-radius: 0; - } - - &-headerGroup { - margin: 0; - } - - &-avatarImage { - width: 24px; - height: 24px; - border-radius: @component-border-radius; - } - - &-title { - font-size: 1.25em; - font-weight: 500; - line-height: 1.3; - } - - &-reactions { - padding-top: @component-padding; - } - - &-buildStatus { - margin: @component-padding 0; - - &:empty { - display: none; - } - } - - .github-PrTimeline { - margin-top: @component-padding; - } - - .timeline-item { - margin: 0; - padding: @component-padding*1.5 0; - } - - .pre-timeline-item-icon { - position: relative; - margin: 0; - &:before { - vertical-align: middle; - } - } - - &-container > .github-DotComMarkdownHtml { - margin: @component-padding 0; - } - -} - -atom-dock .github-PrStatuses { - &-header, - &-list-item { - border-radius: 0; - padding-left: 0; - padding-right: 0; - border-left: none; - border-right: none; - } -} From 12c6df112342fa010ee7bba523caa05bd29c774b Mon Sep 17 00:00:00 2001 From: simurai Date: Tue, 18 Dec 2018 11:02:32 +0900 Subject: [PATCH 096/102] Separate left/right and bottom docks --- styles/dock.less | 139 ++++++++++++++++++++++++----------------------- 1 file changed, 71 insertions(+), 68 deletions(-) diff --git a/styles/dock.less b/styles/dock.less index 525c327b4e..8bdc4a7a3a 100644 --- a/styles/dock.less +++ b/styles/dock.less @@ -42,93 +42,96 @@ atom-dock.bottom { // GitHub Pane ---------------------------------------------- -atom-dock .react-tabs__tab { - padding: none; +atom-dock.left, +atom-dock.right { + .react-tabs__tab { + padding: none; - &--selected { - font-weight: bold; + &--selected { + font-weight: bold; + } } -} -atom-dock .github-IssueishDetailView { - padding: @component-padding; - font-size: @font-size; - background-color: @tool-panel-background-color; + .github-IssueishDetailView { + padding: @component-padding; + font-size: @font-size; + background-color: @tool-panel-background-color; - &-tab { - border: none; - } + &-tab { + border: none; + } - &-tablist { - border-bottom: none; - display: inline; - } + &-tablist { + border-bottom: none; + display: inline; + } - &-header { - margin-bottom: @component-padding*1.5; - padding: 0; - border: none; - border-radius: 0; - } + &-header { + margin-bottom: @component-padding*1.5; + padding: 0; + border: none; + border-radius: 0; + } - &-headerGroup { - margin: 0; - } + &-headerGroup { + margin: 0; + } - &-avatarImage { - width: 24px; - height: 24px; - border-radius: @component-border-radius; - } + &-avatarImage { + width: 24px; + height: 24px; + border-radius: @component-border-radius; + } - &-title { - font-size: 1.25em; - font-weight: 500; - line-height: 1.3; - } + &-title { + font-size: 1.25em; + font-weight: 500; + line-height: 1.3; + } - &-reactions { - padding-top: @component-padding; - } + &-reactions { + padding-top: @component-padding; + } - &-buildStatus { - margin: @component-padding 0; + &-buildStatus { + margin: @component-padding 0; - &:empty { - display: none; + &:empty { + display: none; + } } - } - .github-PrTimeline { - margin-top: @component-padding; - } + .github-PrTimeline { + margin-top: @component-padding; + } - .timeline-item { - margin: 0; - padding: @component-padding*1.5 0; - } + .timeline-item { + margin: 0; + padding: @component-padding*1.5 0; + } - .pre-timeline-item-icon { - position: relative; - margin: 0; - &:before { - vertical-align: middle; + .pre-timeline-item-icon { + position: relative; + margin: 0; + &:before { + vertical-align: middle; + } } - } - &-container > .github-DotComMarkdownHtml { - margin: @component-padding 0; - } + &-container > .github-DotComMarkdownHtml { + margin: @component-padding 0; + } -} + } -atom-dock .github-PrStatuses { - &-header, - &-list-item { - border-radius: 0; - padding-left: 0; - padding-right: 0; - border-left: none; - border-right: none; + .github-PrStatuses { + &-header, + &-list-item { + border-radius: 0; + padding-left: 0; + padding-right: 0; + border-left: none; + border-right: none; + } } } From 8b1055b5753f3c3643f3066101ade67b75f7e9a5 Mon Sep 17 00:00:00 2001 From: simurai Date: Tue, 18 Dec 2018 14:48:04 +0900 Subject: [PATCH 097/102] Restyle PR panes in docks --- styles/dock.less | 130 ++++++++++++++++--------------------- styles/pr-commit-view.less | 3 +- 2 files changed, 59 insertions(+), 74 deletions(-) diff --git a/styles/dock.less b/styles/dock.less index 8bdc4a7a3a..ad2ab5b7da 100644 --- a/styles/dock.less +++ b/styles/dock.less @@ -40,98 +40,82 @@ atom-dock.bottom { } -// GitHub Pane ---------------------------------------------- +// GitHub Pane (in left/right dock) ---------------------------------------------- -atom-dock.left, -atom-dock.right { - .react-tabs__tab { - padding: none; +atom-dock.left .github-IssueishDetailView, +atom-dock.right .github-IssueishDetailView { - &--selected { - font-weight: bold; - } - } - - .github-IssueishDetailView { + // Header + &-header { + display: block; padding: @component-padding; - font-size: @font-size; background-color: @tool-panel-background-color; + } + &-avatar { position: absolute; } + &-avatarImage { + width: 20px; + height: 20px; + } + &-headerRow:nth-child(1) { + padding-left: 25px; // space for avatar + overflow: hidden; + } + &-title { + text-overflow: ellipsis; + white-space: nowrap; + overflow: hidden; + } + &-checkoutButton { + margin: @component-padding 0 0 0; + } - &-tab { - border: none; - } - - &-tablist { - border-bottom: none; - display: inline; - } - - &-header { - margin-bottom: @component-padding*1.5; - padding: 0; - border: none; - border-radius: 0; - } - - &-headerGroup { - margin: 0; - } - - &-avatarImage { - width: 24px; - height: 24px; - border-radius: @component-border-radius; - } - - &-title { - font-size: 1.25em; - font-weight: 500; - line-height: 1.3; - } - - &-reactions { - padding-top: @component-padding; - } - &-buildStatus { - margin: @component-padding 0; + &-tablist { flex-wrap: wrap; } + &-tab { padding: @component-padding/2 @component-padding; } + &-tab-icon { display: none; } - &:empty { - display: none; - } - } - .github-PrTimeline { - margin-top: @component-padding; - } + // [Overview] + &-overview { + font-size: .9em; + & > .github-DotComMarkdownHtml, .timeline-item { - margin: 0; - padding: @component-padding*1.5 0; - } - - .pre-timeline-item-icon { - position: relative; - margin: 0; - &:before { - vertical-align: middle; - } + padding: @component-padding; } - - &-container > .github-DotComMarkdownHtml { - margin: @component-padding 0; - } - } + + // [Build Status] + &-buildStatus { padding: 0; } .github-PrStatuses { + &-header { border-top: none; } &-header, &-list-item { border-radius: 0; - padding-left: 0; - padding-right: 0; border-left: none; border-right: none; } } + + + // [Commits] + .github-PrCommitsView-commitWrapper { + padding: 0; + } + .github-PrCommitView-container { + font-size: .9em; + border-left: none; + border-right: none; + + &:first-child { + border-top: none; + border-radius: 0; + } + &:last-child { + border-bottom: none; + border-radius: 0; + } + } + } diff --git a/styles/pr-commit-view.less b/styles/pr-commit-view.less index ce2c93caef..f39b4b3e33 100644 --- a/styles/pr-commit-view.less +++ b/styles/pr-commit-view.less @@ -39,9 +39,11 @@ &-messageHeadline.is-button { padding: 0; + margin-right: @default-padding/1.5; background-color: transparent; border: none; cursor: pointer; + text-align: left; color: @text-color-highlight; &:hover, @@ -71,7 +73,6 @@ &-moreButton { border: none; - margin-left: @default-padding/1.5; padding: 0em .2em; color: @text-color-subtle; font-style: italic; From 41f4045c32c7aadc8385e9353616b192fa679624 Mon Sep 17 00:00:00 2001 From: simurai Date: Tue, 18 Dec 2018 15:11:39 +0900 Subject: [PATCH 098/102] Style Commit Detail in dock --- styles/dock.less | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/styles/dock.less b/styles/dock.less index ad2ab5b7da..016e30f63d 100644 --- a/styles/dock.less +++ b/styles/dock.less @@ -119,3 +119,14 @@ atom-dock.right .github-IssueishDetailView { } } + + +// Commit Detail (in left/right dock) ---------------------------------------------- + +atom-dock.left .github-CommitDetailView, +atom-dock.right .github-CommitDetailView { + &-header { background-color: @tool-panel-background-color; } + &-commit { padding: @component-padding @component-padding 0 @component-padding; } + &-meta { margin-bottom: @component-padding; } + &-moreText { padding: @component-padding 0; } +} From 412cfae3dd823936b5fcbd912011b34d8895e3b2 Mon Sep 17 00:00:00 2001 From: simurai Date: Tue, 18 Dec 2018 19:47:28 +0900 Subject: [PATCH 099/102] Fix styling for issues --- lib/views/issue-detail-view.js | 40 ++++++++++++++++---------------- styles/issueish-detail-view.less | 15 +++++++++--- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/lib/views/issue-detail-view.js b/lib/views/issue-detail-view.js index 36842041fe..285e1e5344 100644 --- a/lib/views/issue-detail-view.js +++ b/lib/views/issue-detail-view.js @@ -71,7 +71,7 @@ export class BareIssueDetailView extends React.Component { renderIssueBody(issue) { return ( - +
No description provided.'} switchToIssueish={this.props.switchToIssueish} @@ -81,7 +81,7 @@ export class BareIssueDetailView extends React.Component { issue={issue} switchToIssueish={this.props.switchToIssueish} /> - +
); } @@ -93,31 +93,30 @@ export class BareIssueDetailView extends React.Component {
-
+ {this.renderIssueBody(issue)}