From 0679453b35ed71bc2dd943cf0b7b3c0a22932753 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 12:48:25 -0700 Subject: [PATCH 01/32] Add vertical ellipses for context menu --- lib/views/issueish-list-view.js | 3 +++ styles/issueish-list-view.less | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index bf869cebab..563368ee35 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -94,6 +94,9 @@ export default class IssueishListView extends React.Component { displayStyle="short" className="github-IssueishList-item github-IssueishList-item--age" /> + ); } diff --git a/styles/issueish-list-view.less b/styles/issueish-list-view.less index 107eea01d9..843ede95cc 100644 --- a/styles/issueish-list-view.less +++ b/styles/issueish-list-view.less @@ -63,6 +63,11 @@ text-align: center; } + &--menu { + color: @text-color-subtle; + transform: rotate(90deg); + } + } &-more { From 4bdaace6ab461e35443e82897db31b095b17be64 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 12:49:17 -0700 Subject: [PATCH 02/32] Lift logic for opening ReviewItem up to IssueishSearchesController --- lib/controllers/issueish-searches-controller.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/controllers/issueish-searches-controller.js b/lib/controllers/issueish-searches-controller.js index 25c8dd48cc..6fe7f8f57e 100644 --- a/lib/controllers/issueish-searches-controller.js +++ b/lib/controllers/issueish-searches-controller.js @@ -9,6 +9,7 @@ import Search from '../models/search'; import IssueishSearchContainer from '../containers/issueish-search-container'; import CurrentPullRequestContainer from '../containers/current-pull-request-container'; import IssueishDetailItem from '../items/issueish-detail-item'; +import ReviewsItem from '../items/reviews-item'; import {addEvent} from '../reporter-proxy'; export default class IssueishSearchesController extends React.Component { @@ -68,6 +69,7 @@ export default class IssueishSearchesController extends React.Component { workspace={this.props.workspace} workingDirectory={this.props.workingDirectory} onOpenIssueish={this.onOpenIssueish} + onOpenReviews={this.onOpenReviews} onCreatePr={this.props.onCreatePr} /> {this.state.searches.map(search => ( @@ -81,12 +83,27 @@ export default class IssueishSearchesController extends React.Component { onOpenIssueish={this.onOpenIssueish} onOpenSearch={this.onOpenSearch} + onOpenReviews={this.onOpenReviews} /> ))} ); } + onOpenReviews = issueish => { + const uri = ReviewsItem.buildURI({ + host: this.props.endpoint.getHost(), + owner: this.props.remote.getOwner(), + repo: this.props.remote.getRepo(), + number: issueish.getNumber(), + workdir: this.props.workingDirectory, + }); + return this.props.workspace.open(uri).then(() => { + // TODO: specify if from button or context menu? + addEvent('open-reviews-tab', {package: 'github', from: this.constructor.name}); + }); + } + onOpenIssueish = issueish => { return this.props.workspace.open( IssueishDetailItem.buildURI({ From 7cc3fab419a4fb12723a28c28f0c045568441611 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 12:52:00 -0700 Subject: [PATCH 03/32] Pass down onOpenReviews callback as prop to IssueishListView --- lib/containers/current-pull-request-container.js | 4 ++-- lib/containers/issueish-search-container.js | 1 + lib/controllers/issueish-list-controller.js | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/containers/current-pull-request-container.js b/lib/containers/current-pull-request-container.js index 4878222f79..e83a0bac4a 100644 --- a/lib/containers/current-pull-request-container.js +++ b/lib/containers/current-pull-request-container.js @@ -147,7 +147,6 @@ export default class CurrentPullRequestContainer extends React.Component { results={associatedPullRequests.nodes} isLoading={false} workspace={this.props.workspace} - workingDirectory={this.props.workingDirectory} endpoint={this.props.endpoint} resultFilter={issueish => issueish.getHeadRepositoryID() === this.props.repository.id} {...this.controllerProps()} @@ -176,9 +175,10 @@ export default class CurrentPullRequestContainer extends React.Component { return { title: 'Checked out pull request', onOpenIssueish: this.props.onOpenIssueish, + onOpenReviews: this.props.onOpenReviews, emptyComponent: this.renderEmptyTile, needReviewsButton: true, - workingDirectory: this.props.workingDirectory, + workdir: this.props.workingDirectory, }; } } diff --git a/lib/containers/issueish-search-container.js b/lib/containers/issueish-search-container.js index 4566ccec3e..85ad3239bf 100644 --- a/lib/containers/issueish-search-container.js +++ b/lib/containers/issueish-search-container.js @@ -116,6 +116,7 @@ export default class IssueishSearchContainer extends React.Component { title: this.props.search.getName(), onOpenIssueish: this.props.onOpenIssueish, + onOpenReviews: this.props.onOpenReviews, onOpenMore: () => this.props.onOpenSearch(this.props.search), }; } diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index a004632a9f..ad4fa4cf86 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -56,7 +56,7 @@ export class BareIssueishListController extends React.Component { emptyComponent: PropTypes.func, workspace: PropTypes.object, endpoint: EndpointPropType, - workingDirectory: PropTypes.string, + workdir: PropTypes.string.isRequired, needReviewsButton: PropTypes.bool, }; @@ -118,7 +118,7 @@ export class BareIssueishListController extends React.Component { needReviewsButton={this.props.needReviewsButton} onIssueishClick={this.props.onOpenIssueish} onMoreClick={this.props.onOpenMore} - openReviews={this.openReviews} + openReviews={this.props.onOpenReviews} emptyComponent={this.props.emptyComponent} /> ); From 3e8a6c4b9957a67ebd2a2302c631f3e176a1c083 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 12:54:37 -0700 Subject: [PATCH 04/32] Add menu with "See reviews" item --- lib/views/issueish-list-view.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index 563368ee35..a59c7b66db 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -1,5 +1,7 @@ import React, {Fragment} from 'react'; import PropTypes from 'prop-types'; +import {remote} from 'electron'; +const {Menu, MenuItem} = remote; import {autobind} from '../helpers'; import {IssueishPropType} from '../prop-types'; @@ -68,9 +70,18 @@ export default class IssueishListView extends React.Component { ); } - openReviews = e => { - e.stopPropagation(); - this.props.openReviews(); + showActionsMenu(event, issueish) { + event.preventDefault(); + event.stopPropagation(); + + const menu = new Menu(); + + menu.append(new MenuItem({ + label: 'See reviews', + click: () => this.props.openReviews(issueish), + })); + + menu.popup(remote.getCurrentWindow()); } renderIssueish(issueish) { @@ -96,6 +107,7 @@ export default class IssueishListView extends React.Component { /> this.showActionsMenu(event, issueish)} /> ); From ec08555a11fe8ba7b2a2c59539a7d0910f70f065 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 12:55:45 -0700 Subject: [PATCH 05/32] Make "See reviews" button call passed down prop --- lib/views/issueish-list-view.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index a59c7b66db..3046366f09 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -58,13 +58,16 @@ export default class IssueishListView extends React.Component { } renderReviewsButton = () => { - if (!this.props.needReviewsButton || this.props.total < 1) { + if (!this.props.needReviewsButton || this.props.issueishes.length < 1) { return null; } return ( ); From 7ee6f11e5ac402071f2ad07928805fdd329df8e3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 12:56:09 -0700 Subject: [PATCH 06/32] :fire: unused openReviews in IssueishListController --- lib/controllers/issueish-list-controller.js | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index ad4fa4cf86..72ade27a53 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -90,23 +90,6 @@ export class BareIssueishListController extends React.Component { return null; } - openReviews = async () => { - /* istanbul ignore next */ - if (this.props.results.length < 1) { - return; - } - const result = this.props.results[0]; - const uri = ReviewsItem.buildURI({ - host: this.props.endpoint.getHost(), - owner: result.repository.owner.login, - repo: result.repository.name, - number: result.number, - workdir: this.props.workingDirectory, - }); - await this.props.workspace.open(uri); - addEvent('open-reviews-tab', {package: 'github', from: this.constructor.name}); - } - render() { return ( Date: Thu, 18 Apr 2019 13:46:47 -0700 Subject: [PATCH 07/32] Add menu item for opening on GitHub --- lib/views/issueish-list-view.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index 3046366f09..6665a38ee5 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -1,6 +1,6 @@ import React, {Fragment} from 'react'; import PropTypes from 'prop-types'; -import {remote} from 'electron'; +import {remote, shell} from 'electron'; const {Menu, MenuItem} = remote; import {autobind} from '../helpers'; @@ -84,9 +84,22 @@ export default class IssueishListView extends React.Component { click: () => this.props.openReviews(issueish), })); + menu.append(new MenuItem({ + label: 'Open on GitHub', + click: () => this.onOpenIssueish(issueish), + })); + menu.popup(remote.getCurrentWindow()); } + onOpenIssueish = issueish => { + return new Promise((resolve, reject) => { + shell.openExternal(issueish.getGitHubURL(), {}, err => { + if (err) { reject(err); } else { resolve(); } + }); + }); + } + renderIssueish(issueish) { return ( From 294412233e9c62db8e55b7a97bfc037aa85bb684 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 17:16:36 -0700 Subject: [PATCH 08/32] Move test for opening review from IssuishListController to IssueishSearchesController --- .../issueish-list-controller.test.js | 19 -------------- .../issueish-searches-controller.test.js | 25 +++++++++++++++++++ 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/test/controllers/issueish-list-controller.test.js b/test/controllers/issueish-list-controller.test.js index ea1eabe7cb..9db4e1c22a 100644 --- a/test/controllers/issueish-list-controller.test.js +++ b/test/controllers/issueish-list-controller.test.js @@ -84,23 +84,4 @@ describe('IssueishListController', function() { assert.strictEqual(view.prop('total'), 5); assert.deepEqual(view.prop('issueishes').map(issueish => issueish.getNumber()), [11, 13, 12]); }); - - it('opens reviews', async function() { - const atomEnv = global.buildAtomEnvironment(); - sinon.stub(atomEnv.workspace, 'open').resolves(); - sinon.stub(reporterProxy, 'addEvent'); - const pr = createPullRequestResult({number: 1337}); - Object.assign(pr.repository, {owner: {login: 'owner'}, name: 'repo'}); - const wrapper = shallow(buildApp({ - workspace: atomEnv.workspace, - endpoint: getEndpoint('github.com'), - results: [pr], - })); - - await wrapper.find('IssueishListView').prop('openReviews')(); - assert.isTrue(atomEnv.workspace.open.called); - assert.isTrue(reporterProxy.addEvent.calledWith('open-reviews-tab', {package: 'github', from: 'BareIssueishListController'})); - - atomEnv.destroy(); - }); }); diff --git a/test/controllers/issueish-searches-controller.test.js b/test/controllers/issueish-searches-controller.test.js index 96f917d3c8..aa93c0c708 100644 --- a/test/controllers/issueish-searches-controller.test.js +++ b/test/controllers/issueish-searches-controller.test.js @@ -115,4 +115,29 @@ describe('IssueishSearchesController', function() { ); assert.isTrue(reporterProxy.addEvent.calledWith('open-issueish-in-pane', {package: 'github', from: 'issueish-list'})); }); + + it('passes a handler to open reviews and reports an event', async function() { + sinon.spy(atomEnv.workspace, 'open'); + + const wrapper = shallow(buildApp()); + const container = wrapper.find('IssueishSearchContainer').at(0); + + const issueish = new Issueish({ + number: 2084, + url: 'https://github.com/atom/github/pull/2084', + author: {login: 'kuychaco', avatarUrl: 'https://avatars3.githubusercontent.com/u/7910250?v=4'}, + createdAt: '2019-04-01T10:00:00', + repository: {id: '0'}, + commits: {nodes: []}, + }); + + sinon.stub(reporterProxy, 'addEvent'); + await container.prop('onOpenReviews')(issueish); + assert.isTrue( + atomEnv.workspace.open.calledWith( + `atom-github://reviews/github.com/atom/github/2084?workdir=${encodeURIComponent(__dirname)}`, + ), + ); + assert.isTrue(reporterProxy.addEvent.calledWith('open-reviews-tab', {package: 'github', from: 'IssueishSearchesController'})); + }); }); From 755214866a922257038ed4b5f33802af77ed8f73 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 17:18:25 -0700 Subject: [PATCH 09/32] Fix failing IsssueisListView test --- test/views/issueish-list-view.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/views/issueish-list-view.test.js b/test/views/issueish-list-view.test.js index b4a94087ee..047a2b0386 100644 --- a/test/views/issueish-list-view.test.js +++ b/test/views/issueish-list-view.test.js @@ -145,7 +145,7 @@ describe('IssueishListView', function() { it('renders review button only if needed', function() { const openReviews = sinon.spy(); - const wrapper = mount(buildApp({total: 1, openReviews})); + const wrapper = mount(buildApp({issueishes: [allGreen], openReviews})); assert.isFalse(wrapper.find('.github-IssueishList-openReviewsButton').exists()); wrapper.setProps({needReviewsButton: true}); From 1aedec4921772c8502974a303c8b3d51d0135dc5 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 18 Apr 2019 17:36:16 -0700 Subject: [PATCH 10/32] :shirt: --- lib/containers/current-pull-request-container.js | 1 + lib/containers/issueish-search-container.js | 1 + lib/controllers/issueish-list-controller.js | 3 +-- test/controllers/issueish-list-controller.test.js | 2 -- 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/containers/current-pull-request-container.js b/lib/containers/current-pull-request-container.js index e83a0bac4a..9e3e023184 100644 --- a/lib/containers/current-pull-request-container.js +++ b/lib/containers/current-pull-request-container.js @@ -42,6 +42,7 @@ export default class CurrentPullRequestContainer extends React.Component { // Actions onOpenIssueish: PropTypes.func.isRequired, + onOpenReviews: PropTypes.func.isRequired, onCreatePr: PropTypes.func.isRequired, } diff --git a/lib/containers/issueish-search-container.js b/lib/containers/issueish-search-container.js index 85ad3239bf..2fc0c083f0 100644 --- a/lib/containers/issueish-search-container.js +++ b/lib/containers/issueish-search-container.js @@ -22,6 +22,7 @@ export default class IssueishSearchContainer extends React.Component { // Action methods onOpenIssueish: PropTypes.func.isRequired, onOpenSearch: PropTypes.func.isRequired, + onOpenReviews: PropTypes.func.isRequired, } static defaultProps = { diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index 72ade27a53..35a20fffd5 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -4,8 +4,6 @@ import {graphql, createFragmentContainer} from 'react-relay'; import {EndpointPropType} from '../prop-types'; import IssueishListView from '../views/issueish-list-view'; import Issueish from '../models/issueish'; -import ReviewsItem from '../items/reviews-item'; -import {addEvent} from '../reporter-proxy'; const StatePropType = PropTypes.oneOf(['EXPECTED', 'PENDING', 'SUCCESS', 'ERROR', 'FAILURE']); @@ -51,6 +49,7 @@ export class BareIssueishListController extends React.Component { resultFilter: PropTypes.func, onOpenIssueish: PropTypes.func.isRequired, + onOpenReviews: PropTypes.func.isRequired, onOpenMore: PropTypes.func, emptyComponent: PropTypes.func, diff --git a/test/controllers/issueish-list-controller.test.js b/test/controllers/issueish-list-controller.test.js index 9db4e1c22a..8c795424f2 100644 --- a/test/controllers/issueish-list-controller.test.js +++ b/test/controllers/issueish-list-controller.test.js @@ -4,8 +4,6 @@ import {shallow} from 'enzyme'; import {createPullRequestResult} from '../fixtures/factories/pull-request-result'; import Issueish from '../../lib/models/issueish'; import {BareIssueishListController} from '../../lib/controllers/issueish-list-controller'; -import {getEndpoint} from '../../lib/models/endpoint'; -import * as reporterProxy from '../../lib/reporter-proxy'; describe('IssueishListController', function() { From 9afa98159b7911be6b5ebacf67a1432329da9498 Mon Sep 17 00:00:00 2001 From: annthurium Date: Fri, 19 Apr 2019 08:36:04 -0700 Subject: [PATCH 11/32] fix icon vertical centering also remove rotation to be consistent with other kebab menus in the package --- lib/containers/issueish-search-container.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/containers/issueish-search-container.js b/lib/containers/issueish-search-container.js index 2fc0c083f0..071184d48d 100644 --- a/lib/containers/issueish-search-container.js +++ b/lib/containers/issueish-search-container.js @@ -119,6 +119,7 @@ export default class IssueishSearchContainer extends React.Component { onOpenIssueish: this.props.onOpenIssueish, onOpenReviews: this.props.onOpenReviews, onOpenMore: () => this.props.onOpenSearch(this.props.search), + workdir: '', }; } } From c6a8138a2622047e6a23df4e3135d55603128479 Mon Sep 17 00:00:00 2001 From: annthurium Date: Fri, 19 Apr 2019 08:37:30 -0700 Subject: [PATCH 12/32] actually fix the icon I staged the wrong file because I wasn't paying attention -- see previous commit. --- lib/containers/issueish-search-container.js | 1 - styles/issueish-list-view.less | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/containers/issueish-search-container.js b/lib/containers/issueish-search-container.js index 071184d48d..2fc0c083f0 100644 --- a/lib/containers/issueish-search-container.js +++ b/lib/containers/issueish-search-container.js @@ -119,7 +119,6 @@ export default class IssueishSearchContainer extends React.Component { onOpenIssueish: this.props.onOpenIssueish, onOpenReviews: this.props.onOpenReviews, onOpenMore: () => this.props.onOpenSearch(this.props.search), - workdir: '', }; } } diff --git a/styles/issueish-list-view.less b/styles/issueish-list-view.less index 843ede95cc..f79e7b4640 100644 --- a/styles/issueish-list-view.less +++ b/styles/issueish-list-view.less @@ -65,7 +65,7 @@ &--menu { color: @text-color-subtle; - transform: rotate(90deg); + line-height: 1; } } From 5387610747b4a129348251dd11f1b5ec757d0e23 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 19 Apr 2019 10:51:15 -0700 Subject: [PATCH 13/32] Istanbul ignore electron menu logic Seems like there's no good way to test Electron menus. Not even supported in spectron -- https://github.com/electron/spectron/issues/21 --- lib/views/issueish-list-view.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index 6665a38ee5..43b5bc6fbf 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -73,6 +73,7 @@ export default class IssueishListView extends React.Component { ); } + /* istanbul ignore next */ showActionsMenu(event, issueish) { event.preventDefault(); event.stopPropagation(); From 515f91c575883551cadbfd62d54dfe6a4a1babfc Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 22 Apr 2019 16:48:04 -0700 Subject: [PATCH 14/32] Move logic to open issue on GitHub from view to controller --- lib/controllers/issueish-list-controller.js | 10 ++++++++++ lib/views/issueish-list-view.js | 12 ++---------- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index 35a20fffd5..b44b055355 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -4,6 +4,7 @@ import {graphql, createFragmentContainer} from 'react-relay'; import {EndpointPropType} from '../prop-types'; import IssueishListView from '../views/issueish-list-view'; import Issueish from '../models/issueish'; +import {shell} from 'electron'; const StatePropType = PropTypes.oneOf(['EXPECTED', 'PENDING', 'SUCCESS', 'ERROR', 'FAILURE']); @@ -89,6 +90,14 @@ export class BareIssueishListController extends React.Component { return null; } + openOnGitHub = issueish => { + return new Promise((resolve, reject) => { + shell.openExternal(issueish.getGitHubURL(), {}, err => { + if (err) { reject(err); } else { resolve(); } + }); + }); + } + render() { return ( ); diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index 43b5bc6fbf..f04e7c8b65 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -1,6 +1,6 @@ import React, {Fragment} from 'react'; import PropTypes from 'prop-types'; -import {remote, shell} from 'electron'; +import {remote} from 'electron'; const {Menu, MenuItem} = remote; import {autobind} from '../helpers'; @@ -87,20 +87,12 @@ export default class IssueishListView extends React.Component { menu.append(new MenuItem({ label: 'Open on GitHub', - click: () => this.onOpenIssueish(issueish), + click: () => this.props.openOnGitHub(issueish), })); menu.popup(remote.getCurrentWindow()); } - onOpenIssueish = issueish => { - return new Promise((resolve, reject) => { - shell.openExternal(issueish.getGitHubURL(), {}, err => { - if (err) { reject(err); } else { resolve(); } - }); - }); - } - renderIssueish(issueish) { return ( From 2e47609495acbb8116e2b05d0e4a10bb5e261c6b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:11:56 -0700 Subject: [PATCH 15/32] Add `open-issueish-in-browser` event and pass url instead of issueish --- lib/controllers/issueish-list-controller.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index b44b055355..68a02cd0d6 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -5,6 +5,7 @@ import {EndpointPropType} from '../prop-types'; import IssueishListView from '../views/issueish-list-view'; import Issueish from '../models/issueish'; import {shell} from 'electron'; +import {addEvent} from '../reporter-proxy'; const StatePropType = PropTypes.oneOf(['EXPECTED', 'PENDING', 'SUCCESS', 'ERROR', 'FAILURE']); @@ -90,10 +91,13 @@ export class BareIssueishListController extends React.Component { return null; } - openOnGitHub = issueish => { + openOnGitHub = url => { return new Promise((resolve, reject) => { - shell.openExternal(issueish.getGitHubURL(), {}, err => { - if (err) { reject(err); } else { resolve(); } + shell.openExternal(url, {}, err => { + if (err) { reject(err); } else { + resolve(); + addEvent('open-issueish-in-browser', {package: 'github', component: this.constructor.name}); + } }); }); } From 65e95e33d6cfa24bc788deb49bf339030ff865b3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:12:49 -0700 Subject: [PATCH 16/32] Move `showActionsMenu` to IssueishListController Keep view simple and free of unnecessary logic --- lib/controllers/issueish-list-controller.js | 20 ++++++++++++++- lib/views/issueish-list-view.js | 27 ++++++--------------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index 68a02cd0d6..a95dd74925 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -4,7 +4,8 @@ import {graphql, createFragmentContainer} from 'react-relay'; import {EndpointPropType} from '../prop-types'; import IssueishListView from '../views/issueish-list-view'; import Issueish from '../models/issueish'; -import {shell} from 'electron'; +import {shell, remote} from 'electron'; +const {Menu, MenuItem} = remote; import {addEvent} from '../reporter-proxy'; const StatePropType = PropTypes.oneOf(['EXPECTED', 'PENDING', 'SUCCESS', 'ERROR', 'FAILURE']); @@ -102,6 +103,23 @@ export class BareIssueishListController extends React.Component { }); } + /* istanbul ignore next */ + showActionsMenu(event, issueish) { + const menu = new Menu(); + + menu.append(new MenuItem({ + label: 'See reviews', + click: () => this.props.onOpenReviews(issueish), + })); + + menu.append(new MenuItem({ + label: 'Open on GitHub', + click: () => this.openOnGitHub(issueish.getGitHubURL()), + })); + + menu.popup(remote.getCurrentWindow()); + } + render() { return ( this.props.openReviews(issueish), - })); - - menu.append(new MenuItem({ - label: 'Open on GitHub', - click: () => this.props.openOnGitHub(issueish), - })); - - menu.popup(remote.getCurrentWindow()); - } - renderIssueish(issueish) { return ( @@ -122,6 +102,13 @@ export default class IssueishListView extends React.Component { ); } + showActionsMenu(event, issueish) { + event.preventDefault(); + event.stopPropagation(); + + this.props.showActionsMenu(issueish); + } + renderStatusSummary(statusCounts) { if (['success', 'failure', 'pending'].every(kind => statusCounts[kind] === 0)) { return ; From 2432b84901123ddc058c2e9f64611263c68bd642 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:13:12 -0700 Subject: [PATCH 17/32] :fire: unnecessary imports --- lib/views/issueish-list-view.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index 9a9119cabf..07e0873d08 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -1,7 +1,5 @@ import React, {Fragment} from 'react'; import PropTypes from 'prop-types'; -import {remote} from 'electron'; -const {Menu, MenuItem} = remote; import {autobind} from '../helpers'; import {IssueishPropType} from '../prop-types'; From d74436a21fd25553ae197ad9ae040ab412231926 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:13:37 -0700 Subject: [PATCH 18/32] Add prop types --- lib/views/issueish-list-view.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index 07e0873d08..322b42a488 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -26,7 +26,9 @@ export default class IssueishListView extends React.Component { needReviewsButton: PropTypes.bool, onIssueishClick: PropTypes.func.isRequired, onMoreClick: PropTypes.func, - openReviews: PropTypes.func, + openReviews: PropTypes.func.isRequired, + openOnGitHub: PropTypes.func.isRequired, + showActionsMenu: PropTypes.func.isRequired, emptyComponent: PropTypes.func, error: PropTypes.object, From 47312af92f56824afd81106da43b93de46cbbb92 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:13:55 -0700 Subject: [PATCH 19/32] :fire: unused prop types --- lib/containers/current-pull-request-container.js | 2 -- lib/controllers/issueish-list-controller.js | 1 - 2 files changed, 3 deletions(-) diff --git a/lib/containers/current-pull-request-container.js b/lib/containers/current-pull-request-container.js index 9e3e023184..6bccb589df 100644 --- a/lib/containers/current-pull-request-container.js +++ b/lib/containers/current-pull-request-container.js @@ -38,7 +38,6 @@ export default class CurrentPullRequestContainer extends React.Component { pushInProgress: PropTypes.bool.isRequired, workspace: PropTypes.object.isRequired, - workingDirectory: PropTypes.string.isRequired, // Actions onOpenIssueish: PropTypes.func.isRequired, @@ -179,7 +178,6 @@ export default class CurrentPullRequestContainer extends React.Component { onOpenReviews: this.props.onOpenReviews, emptyComponent: this.renderEmptyTile, needReviewsButton: true, - workdir: this.props.workingDirectory, }; } } diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index a95dd74925..ebd34b161f 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -58,7 +58,6 @@ export class BareIssueishListController extends React.Component { emptyComponent: PropTypes.func, workspace: PropTypes.object, endpoint: EndpointPropType, - workdir: PropTypes.string.isRequired, needReviewsButton: PropTypes.bool, }; From 005883e132bb781e53f7575cac1cad5db545b6a2 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:14:35 -0700 Subject: [PATCH 20/32] Test that `showActionsMenu` handler is called when octicon is clicked --- test/views/issueish-list-view.test.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/views/issueish-list-view.test.js b/test/views/issueish-list-view.test.js index 047a2b0386..f731497282 100644 --- a/test/views/issueish-list-view.test.js +++ b/test/views/issueish-list-view.test.js @@ -141,6 +141,20 @@ describe('IssueishListView', function() { wrapper.find('.github-IssueishList-more a').simulate('click'); assert.isTrue(onMoreClick.called); }); + + it('calls its `showActionsMenu` handler when the menu icon is clicked', function() { + const issueishes = [allGreen, mixed, allRed]; + const showActionsMenu = sinon.stub(); + const wrapper = mount(buildApp({ + isLoading: false, + total: 3, + issueishes, + showActionsMenu, + })); + + wrapper.find('Octicon.github-IssueishList-item--menu').at(1).simulate('click'); + assert.isTrue(showActionsMenu.calledWith(mixed)); + }); }); it('renders review button only if needed', function() { From 7ea7c39a9689a251964b44ffd749ae925f0be449 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:15:30 -0700 Subject: [PATCH 21/32] Add test for `openOnGitHub` --- test/controllers/issueish-list-controller.test.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/controllers/issueish-list-controller.test.js b/test/controllers/issueish-list-controller.test.js index 8c795424f2..bf2c7c0cbf 100644 --- a/test/controllers/issueish-list-controller.test.js +++ b/test/controllers/issueish-list-controller.test.js @@ -1,5 +1,6 @@ import React from 'react'; import {shallow} from 'enzyme'; +import {shell} from 'electron'; import {createPullRequestResult} from '../fixtures/factories/pull-request-result'; import Issueish from '../../lib/models/issueish'; @@ -82,4 +83,18 @@ describe('IssueishListController', function() { assert.strictEqual(view.prop('total'), 5); assert.deepEqual(view.prop('issueishes').map(issueish => issueish.getNumber()), [11, 13, 12]); }); + + describe('openOnGitHub', function() { + const url = 'https://github.com/atom/github/pull/2084'; + + it('calls shell.openExternal with specified url', async function() { + const wrapper = shallow(buildApp()); + sinon.stub(shell, 'openExternal').callsArg(2); + + await wrapper.instance().openOnGitHub(url); + assert.equal(shell.openExternal.args[0][0], url); + }); + }); + }); + }); From 8d113d11a78cdebdf4dcf018b1f3a48bfabfd1b1 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:15:55 -0700 Subject: [PATCH 22/32] Add test for `open-issueish-in-browser` event --- .../issueish-list-controller.test.js | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/controllers/issueish-list-controller.test.js b/test/controllers/issueish-list-controller.test.js index bf2c7c0cbf..47a7a2193a 100644 --- a/test/controllers/issueish-list-controller.test.js +++ b/test/controllers/issueish-list-controller.test.js @@ -5,6 +5,7 @@ import {shell} from 'electron'; import {createPullRequestResult} from '../fixtures/factories/pull-request-result'; import Issueish from '../../lib/models/issueish'; import {BareIssueishListController} from '../../lib/controllers/issueish-list-controller'; +import * as reporterProxy from '../../lib/reporter-proxy'; describe('IssueishListController', function() { @@ -94,6 +95,29 @@ describe('IssueishListController', function() { await wrapper.instance().openOnGitHub(url); assert.equal(shell.openExternal.args[0][0], url); }); + + it('fires `open-issueish-in-browser` event upon success', async function() { + const wrapper = shallow(buildApp()); + sinon.stub(shell, 'openExternal').callsArg(2); + sinon.stub(reporterProxy, 'addEvent'); + + await wrapper.instance().openOnGitHub(url); + assert.equal(reporterProxy.addEvent.callCount, 1); + + await assert.isTrue(reporterProxy.addEvent.calledWith('open-issueish-in-browser', {package: 'github', component: 'BareIssueishListController'})); + }); + + it('handles error when openOnGitHub fails', async function() { + const wrapper = shallow(buildApp()); + sinon.stub(shell, 'openExternal').callsArgWith(2, new Error('oh noes')); + sinon.stub(reporterProxy, 'addEvent'); + + try { + await wrapper.instance().openOnGitHub(url); + } catch (err) { + assert.equal(err.message, 'oh noes'); + } + assert.equal(reporterProxy.addEvent.callCount, 0); }); }); From 9c5adda957e3476dc32d0075cd40e2b8b92110c7 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:18:36 -0700 Subject: [PATCH 23/32] :fire: todo comment --- lib/controllers/issueish-searches-controller.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/controllers/issueish-searches-controller.js b/lib/controllers/issueish-searches-controller.js index 6fe7f8f57e..b991fb0a7e 100644 --- a/lib/controllers/issueish-searches-controller.js +++ b/lib/controllers/issueish-searches-controller.js @@ -99,7 +99,6 @@ export default class IssueishSearchesController extends React.Component { workdir: this.props.workingDirectory, }); return this.props.workspace.open(uri).then(() => { - // TODO: specify if from button or context menu? addEvent('open-reviews-tab', {package: 'github', from: this.constructor.name}); }); } From 06468727c50e2fe9f3c21829bb087f0688494285 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:24:38 -0700 Subject: [PATCH 24/32] Make click handler bound class method --- lib/views/issueish-list-view.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/views/issueish-list-view.js b/lib/views/issueish-list-view.js index 322b42a488..8e1d33c9a6 100644 --- a/lib/views/issueish-list-view.js +++ b/lib/views/issueish-list-view.js @@ -64,15 +64,17 @@ export default class IssueishListView extends React.Component { return ( ); } + openReviews = e => { + e.stopPropagation(); + this.props.openReviews(this.props.issueishes[0]); + } + renderIssueish(issueish) { return ( From ea1839584df419e062307c64f3bdeca3d14a4439 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:27:17 -0700 Subject: [PATCH 25/32] Oops, actually pass down `showActionsMenu` to the view --- lib/controllers/issueish-list-controller.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index ebd34b161f..3deeaef2b0 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -132,6 +132,7 @@ export class BareIssueishListController extends React.Component { onMoreClick={this.props.onOpenMore} openReviews={this.props.onOpenReviews} openOnGitHub={this.openOnGitHub} + showActionsMenu={this.showActionsMenu} emptyComponent={this.props.emptyComponent} /> ); From 7f7565c92a1735e99d748cd4e764759e2a3e716c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:29:44 -0700 Subject: [PATCH 26/32] Oops, showActionsMenu needs to be an arrow function to bind `this` --- lib/controllers/issueish-list-controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index 3deeaef2b0..9882eecff2 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -103,7 +103,7 @@ export class BareIssueishListController extends React.Component { } /* istanbul ignore next */ - showActionsMenu(event, issueish) { + showActionsMenu = (event, issueish) => { const menu = new Menu(); menu.append(new MenuItem({ From f119527f6c8930c0ba2159e8911ececed42b858e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 13:33:36 -0700 Subject: [PATCH 27/32] Fix parameters for `showActionsMenu` --- lib/controllers/issueish-list-controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index 9882eecff2..65858bebbf 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -103,7 +103,7 @@ export class BareIssueishListController extends React.Component { } /* istanbul ignore next */ - showActionsMenu = (event, issueish) => { + showActionsMenu = issueish => { const menu = new Menu(); menu.append(new MenuItem({ From dc25f9d4bf5b2b81763dafcc0f3c735810f7f46f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 14:11:29 -0700 Subject: [PATCH 28/32] Use `istanbul ignore next` correctly Co-Authored-By: Tilde Ann Thurium --- lib/controllers/issueish-list-controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index 65858bebbf..af0f9d1a98 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -102,8 +102,8 @@ export class BareIssueishListController extends React.Component { }); } - /* istanbul ignore next */ showActionsMenu = issueish => { + /* istanbul ignore next */ const menu = new Menu(); menu.append(new MenuItem({ From 717f3e12fd4965086a5803f46716c28ce46ccecd Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 23 Apr 2019 16:23:58 -0700 Subject: [PATCH 29/32] Okay, _actually_ istanbul-ignore method this time --- lib/controllers/issueish-list-controller.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/controllers/issueish-list-controller.js b/lib/controllers/issueish-list-controller.js index af0f9d1a98..5903a9ed71 100644 --- a/lib/controllers/issueish-list-controller.js +++ b/lib/controllers/issueish-list-controller.js @@ -102,8 +102,7 @@ export class BareIssueishListController extends React.Component { }); } - showActionsMenu = issueish => { - /* istanbul ignore next */ + showActionsMenu = /* istanbul ignore next */ issueish => { const menu = new Menu(); menu.append(new MenuItem({ From ff2056749b4f9fe708b17a04a73100627368ca7a Mon Sep 17 00:00:00 2001 From: Vanessa Yuen <6842965+vanessayuenn@users.noreply.github.com> Date: Thu, 25 Apr 2019 15:53:49 -0700 Subject: [PATCH 30/32] Update test/controllers/issueish-list-controller.test.js Co-Authored-By: kuychaco --- test/controllers/issueish-list-controller.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/issueish-list-controller.test.js b/test/controllers/issueish-list-controller.test.js index 47a7a2193a..f2f5999e1b 100644 --- a/test/controllers/issueish-list-controller.test.js +++ b/test/controllers/issueish-list-controller.test.js @@ -93,7 +93,7 @@ describe('IssueishListController', function() { sinon.stub(shell, 'openExternal').callsArg(2); await wrapper.instance().openOnGitHub(url); - assert.equal(shell.openExternal.args[0][0], url); + assert.isTrue(shell.openExternal.calledWith(url)); }); it('fires `open-issueish-in-browser` event upon success', async function() { From 62b2b89b4486831af3c18543aacb71809dbcf446 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 25 Apr 2019 15:57:32 -0700 Subject: [PATCH 31/32] Use strictEqual instead of equal Co-Authored-By: Vanessa Yuen --- test/controllers/issueish-list-controller.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/controllers/issueish-list-controller.test.js b/test/controllers/issueish-list-controller.test.js index f2f5999e1b..c5b7ab4e3d 100644 --- a/test/controllers/issueish-list-controller.test.js +++ b/test/controllers/issueish-list-controller.test.js @@ -102,7 +102,7 @@ describe('IssueishListController', function() { sinon.stub(reporterProxy, 'addEvent'); await wrapper.instance().openOnGitHub(url); - assert.equal(reporterProxy.addEvent.callCount, 1); + assert.strictEqual(reporterProxy.addEvent.callCount, 1); await assert.isTrue(reporterProxy.addEvent.calledWith('open-issueish-in-browser', {package: 'github', component: 'BareIssueishListController'})); }); @@ -115,9 +115,9 @@ describe('IssueishListController', function() { try { await wrapper.instance().openOnGitHub(url); } catch (err) { - assert.equal(err.message, 'oh noes'); + assert.strictEqual(err.message, 'oh noes'); } - assert.equal(reporterProxy.addEvent.callCount, 0); + assert.strictEqual(reporterProxy.addEvent.callCount, 0); }); }); From cff5a35a044d61df4dbb832884aa57dae8795c65 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 26 Apr 2019 12:17:52 -0700 Subject: [PATCH 32/32] Empty commit to kick off CI after setup got messed up