Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull Request list #1523

Merged
merged 146 commits into from Jun 22, 2018

Conversation

Projects
6 participants
@smashwilson
Copy link
Member

smashwilson commented Jun 12, 2018

Implement the first part of RFC-002: two lists of pull requests in the GitHub tab, one showing the "current" pull request (if any), and the other showing all pull requests on the chosen remote.

Before + After
GitHub tab: before GitHub tab: after
before screen shot 2018-06-21 at 1 13 47 pm

GitHub tab: after, on default ref

after-default-ref

GitHub tab: after, on unpublished ref

after-publish-create

The "current pull request" list displays any pull requests that are associated with the ref that HEAD pushes to that are also targeted at the chosen remote.

Special handling also exists for other cases:

  • When HEAD is a detached ref (like a tag);
  • When no repository exists for the push remote of the HEAD ref;
  • When the GitHub repository has no default ref (before the first push);
  • A HEAD ref that's not on the default branch, but that hasn't moved from it;
  • When a push is in progress;
  • When an upstream ref exists, but the local ref has unpushed commits;
  • When the upstream ref exists and is up to date, but has no associated pull request.
Component hierarchy
  • GithubTabController pane item methods; fetch async data from repository model; top-level tab actions
    • RemoteContainer fetch the appropriate token from the login model; query GraphQL for data about the chosen remote; render login, loading, or error views when appropriate
      • RemoteController createPR action method
        • IssueishSearchesController manage set of Search models
          • IssueishSearchContainer perform GraphQL query for a single search. handle loading and error views.
            • IssueishListController translate GraphQL result objects into Issueish models
              • IssueishListView render Accordion widget with issueish results
          • CurrentPullRequestContainer perform GraphQL query for current PR. handle loading and error views.
            • IssueishListController translate GraphQL result objects into Issueish models
              • IssueishListView render Accordion widget with issueish results
Countdown to 馃殌
  • Show the "create a pull request" tile on the "Current pull request" accordion when empty
  • Fetch and display the real build status
  • Render the real issueish relative age
  • Open an IssueishPaneItem on result click
  • Render "More..." link when list is truncated
  • Rename RemotePrController
  • Allow IssueishPaneItems to be pending
  • Handle QueryRenderer errors within an IssueishListContainer
  • Refresh issueish queries
    • On remote git operations
  • Investigate backing the "current" PR with an associatedPullRequest from the upstream ref.
  • 馃帹 Center the loading spinner in the Github tab again.
  • Clean up and consistency
    • Consistently use tests/factories/pull-request-result factory
    • Delete now-unused components
    • Remove unused CSS
  • Add differentiation icons to each message in CreatePullRequestTile
  • Add newlines between sentences in CreatePullRequestTile messages
  • Investigate de-emphasizing bolded messages with a different kind of highlighting method that doesn't look as clickable
  • Re-word the onDefaultRef message

smashwilson and others added some commits Jun 12, 2018

Add example staus and age
Still needs wiring up
<Fragment>
<img
className="github-IssueishList-item--avatar"
src={issueish.getAuthorAvatarURL() + "&s=32"}

This comment has been minimized.

@simurai

simurai Jun 14, 2018

Member

I limited the avatar size to 32px. Makes it look sharper and probably needs less memory?

It currently works, but not sure if this is a good way to set the size. I guess it only works if the URL has something like ?v=4 at the end.

This comment has been minimized.

@smashwilson

smashwilson Jun 14, 2018

Author Member

Ah right cool. I've added a size parameter to the Issueish.getAuthorAvatarURL() method and used URL manipulation methods to add the query string parameter... just in case.


render() {
return (
<details className="github-Accordion" open={this.state.expanded}>

This comment was marked as resolved.

@simurai

simurai Jun 14, 2018

Member

This seems to not work for the first click?

  1. Reload Atom -> open is present
  2. Click on the header -> open stays present
  3. Click again on the header -> open gets removed

open

This comment was marked as resolved.

@smashwilson

smashwilson Jun 14, 2018

Author Member

Ahhh good eye. Looks like I was missing a .preventDefault() call. (I want to manage the Accordion's expanded state in React rather than the DOM, because then I can not bother rendering a closed Accordion's children if they won't be visible.)

This comment has been minimized.

@simurai

simurai Jun 15, 2018

Member

We can also move away from the <details> element and just use a <div> or so? Then the triangle icon can be custom.

This comment has been minimized.

@smashwilson

smashwilson Jun 15, 2018

Author Member

Oh, sure. I do like keeping markup semantic when I get the chance and not just using <div> every time. But if it's more convenient we can make the elements whatever's easiest.

smashwilson added some commits Jun 14, 2018

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jun 14, 2018

馃槏

assert.isTrue(wrapper.find('span.github-IssueishList-item--status').hasClass('icon-x'));
});

it('renders a donut chart if status checks are mixed', function() {

This comment has been minimized.

@annthurium

annthurium Jun 20, 2018

Contributor

image

Please let me know if you get tired/annoyed of the bitmoji thing, I can stop.

This comment has been minimized.

@smashwilson

smashwilson Jun 21, 2018

Author Member

Please let me know if you get tired/annoyed of the bitmoji thing, I can stop.

I'll be sure to let you know when it stops making me laugh every time 馃槅

@annthurium
Copy link
Contributor

annthurium left a comment

Looks good! I thought of a few additional test cases you might want to consider. But overall, excellent work!

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jun 21, 2018

it would be great if you could add some before/after screenshots to this pull request.

馃憤 I was wondering why the status icons aren't centered in @smashwilson's screenshots... mystery solved:

icons

@simurai
Copy link
Member

simurai left a comment

馃殺 馃殺 馃殺

smashwilson added some commits Jun 21, 2018

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jun 21, 2018

馃憢
Thank you @smashwilson and @simurai for this awesome progress on the pr review experience in Atom 馃帀

A couple things that I think we could improve before 馃殺

Calling attention to a UI change

When you are on master, Current pull request has the following messaging:
screen shot 2018-06-21 at 3 00 58 pm
And when you create a new branch or switch branches that are in sync with master you see the following:
screen shot 2018-06-21 at 3 00 50 pm
These are extremely similar in look, and nothing happens to call attention to the fact that they have changed. Could we brainstorm a solution to drawing attention to this updated information for the user so that they know what to do next?

(potentially)Fixing messaging

The messaging in the first image above is:
You are currently on your repository's default branch. Create a new branch to share your work with a pull request
However, I see three possible scenarios for a user when they are leaving the master branch:

  1. They are going to a new branch and will potentially open a pr for that branch (what is currently in the messaging)
  2. They are going to an existing branch that is not a part of a pr
  3. They are going to an existing branch that is a part of a pr
    I'm trying to figure out where we want to go eventually with our messaging here. Eventually we want things like what we have in the rfc:

"Checkout" button to fetch (if necessary) and check out the pull request. Only enabled if the current pull request is not the current one.

It would be great to get some mock ups where this entire feature would start. As in, what could we imagine is the entry point to the entire pull request experience?
I would like to continue this conversation in a different Issue, for the purposes of this pr, considering we don't have checkout yet, should we add something to the effect of You are currently on your repository's default branch. Checkout or create a new branch to share your work with a pull request?

Confirmation of scope of this pr

Finally, just to confirm, this pr scope does not include the following from the rfc yet:

  • Branches -> master < aw/rfc-pr-list
  • "Checkout" button to fetch (if necessary) and check out the pull request. Only enabled if the current pull request is not the current one.
  • Commits with count, links to .com (for now), optional with avatars
  • Checks with count, links to .com (for now)
  • CI status, each item links to the detail page
  • Files changed with count, links to .com (for now), optional with "+-" bar
  • Mergability status -> Able to merge, links to the Merging controls at the bottom
  • "Merge PR" to merge the pull request on GitHub if it is open.
  • "Close" to close the pull request, unmerged, if it is open.
  • "Re-open PR" to re-open a pull request if it is closed.
  • Conversation with comment count, opens the current PR timeline in a center pane.
  • Reaction emoji and counts.
  • Description (PR body) as rendered markdown.

That is totally fine, I just wanted to make sure I wasn't missing that :D

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jun 21, 2018

These are extremely similar in look, and nothing happens to call attention to the fact that they have changed

馃憤 Yeah, good point. Maybe we could differentiate them with some iconography on the left... ? For reference, you can see all of the special cases for that tile here:

if (this.isRepositoryNotFound()) {
return (
<div className="github-CreatePullRequestTile-message">
<strong>Repository not found</strong> for the remote <code>{this.props.remote.getName()}</code>.
Do you need to update your remote URL?
</div>
);
}
if (this.isDetachedHead()) {
return (
<div className="github-CreatePullRequestTile-message">
You are not currently on any branch.
&nbsp;<strong>Create a new branch</strong>&nbsp;
to share your work with a pull request.
</div>
);
}
if (this.hasNoDefaultRef()) {
return (
<div className="github-CreatePullRequestTile-message">
The repository at remote <code>{this.props.remote.getName()}</code> is
empty. Push a main branch to begin sharing your work.
</div>
);
}
if (this.isOnDefaultRef()) {
return (
<div className="github-CreatePullRequestTile-message">
You are currently on your repository's default branch.
&nbsp;<strong>Create a new branch</strong>&nbsp;
to share your work with a pull request.
</div>
);
}
if (this.isSameAsDefaultRef()) {
return (
<div className="github-CreatePullRequestTile-message">
Your current branch has not moved from the repository's default branch.
&nbsp;<strong>Make some commits</strong>&nbsp;
to share your work with a pull request.
</div>
);
}
let message = 'Open new pull request';
let disable = false;
const differentRemote = this.pushesToDifferentRemote();
if (this.props.pushInProgress) {
message = 'Pushing...';
disable = true;
} else if (!this.hasUpstreamBranch() || differentRemote) {
message = 'Publish + open new pull request';
} else if (this.props.aheadCount > 0) {
message = 'Push + open new pull request';
}
return (
<div>
{differentRemote &&
<div className="github-CreatePullRequestTile-message">
Your current branch is configured to push to the remote
<code>{this.props.branches.getHeadBranch().getPush().getRemoteName()}</code>.
Publish it to <code>{this.props.remote.getName()}</code> instead?
</div>
}
<p className="github-CreatePullRequestTile-controls">
<button
className="github-CreatePullRequestTile-createPr btn btn-primary"
onClick={this.props.onCreatePr}
disabled={disable}>
{message}
</button>
</p>
</div>
);

By the way: something that I really wanted to do was, rather than just bolding some text, offer a control that lets you do the thing that we're prompting for. The problem is that we can't currently enter the branch creation flow because it's done with a popup. I'm hoping that we can revisit that after #1370 is accepted.

(potentially) Fixing messaging

For a bit of background, I took the messaging verbatim from the old GitHub tab code, which I'd introduced kind of ad-hoc in #1376. Happy to workshop better text with anyone who has ideas for improvements 馃槃 We do have less space in the tile version, so it could likely stand to be made more terse, if nothing else.

However, I see three possible scenarios for a user when they are leaving the master branch:

馃 True, true. Is there some simple way we can say "you have to move off of master for this to be useful"... ? The compare view from master to master would always be empty, which is why we have that particular scenario special-cased to begin with. I'm not sure of an alternative that covers all of the possible next actions without being really wordy.

I'm trying to figure out where we want to go eventually with our messaging here. Eventually we want things like what we have in the rfc:

"Checkout" button to fetch (if necessary) and check out the pull request. Only enabled if the current pull request is not the current one.

That's destined for the issueish pane item at the moment, though. The "current pull request" is populated based on HEAD so checkout would never be a viable action, right?

Finally, just to confirm, this pr scope does not include the following from the rfc yet:

馃憤 Yup yup. This is intended to include everything from the Accordion lists and New PR sections of the RFC. "Issueish Pane Item" is still yet to be done. The pane item I'm opening on click here is entirely pre-existing code.

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jun 21, 2018

Maybe we could differentiate them with some iconography on the left... ?

Yeah! That's a great start! @simurai what do you think?

By the way: something that I really wanted to do was, rather than just bolding some text, offer a control that lets you do the thing that we're prompting for. The problem is that we can't currently enter the branch creation flow because it's done with a popup. I'm hoping that we can revisit that after #1370 is accepted.

I'm not sure of an alternative that covers all of the possible next actions without being really wordy.

Let's brainstorm just a bit but not let it totally block us today

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 22, 2018

Coverage Status

Coverage increased (+1.3%) to 71.353% when pulling 66d149c on aw/accordion-solo into 1fde4dc on master.

@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented Jun 22, 2018

This UI looks so friggin' good

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jun 22, 2018

The messages are now split into two parts:

  1. Why there is no PR.
  2. Possible next step.

The "next step" has an icon as a hint.

image

All messages

Screenshot if
isOnDefaultRef isOnDefaultRef
isSameAsDefaultRef isSameAsDefaultRef
hasNoDefaultRef hasNoDefaultRef
isDetachedHead isDetachedHead
isRepositoryNotFound isRepositoryNotFound
differentRemote differentRemote
default default

Alternatives

I tried italic but it doesn't stick out as much:

image

Since both parts have bold words, it might look less like they are links to click?

/cc @sguthals @annthurium @smashwilson

Fix lint warning
"Trailing spaces not allowed"
@sguthals
Copy link
Contributor

sguthals left a comment

These are @simurai !!!!

Thank you!!!

Looks good to me with these UI changes 馃帀

@smashwilson smashwilson merged commit 1e19579 into master Jun 22, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.3%) to 71.353%
Details

Feature Sprint: 18 June - 6 July 2018 automation moved this from QA Review 馃敩 to Merged 鈽戯笍 Jun 22, 2018

@smashwilson smashwilson deleted the aw/accordion-solo branch Jun 22, 2018

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jun 22, 2018

pshooooo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.