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

RFC for Issueish List View #1503

Merged
merged 18 commits into from Jun 11, 2018

Conversation

Projects
4 participants
@smashwilson
Copy link
Member

smashwilson commented Jun 1, 2018

馃尃 This is based heavily on some excellent internal design ideation by @simurai. 馃尃

I've tried to identify a subset of his vision we could reasonably ship within the next month. My goals here are to rework the GitHub panel to explore an alternate navigation flow (accordion list 馃憠 detail panes) and to set the stage just enough to be able to start on pull request reviews. This will likely not be the final form of "the first pane you see," because we'll probably have at least one level of navigation above it - but we can explore that in follow-on work.

馃帹 rendered 馃帹

/cc @donokuda
/cc @sguthals
/cc @shana

Draft draft draft
Co-Authored-By: simurai <simurai@users.noreply.github.com>
@smashwilson
Copy link
Member Author

smashwilson left a comment

Self-review with 馃挱 thoughts

* Title, truncated if necessary
* Terse relative timestamp (1d, 2h, 30m)
* Pull request state: open, closed, merged
* Status check summary

This comment has been minimized.

@smashwilson

smashwilson Jun 1, 2018

Author Member

鈽濓笍 For this I was thinking a "mini donut view" might be cool. It'd be pretty small and it'd still let you tell the difference between "everything failed," "some stuff was green," and "all good to go".

This comment has been minimized.

@simurai

simurai Jun 4, 2018

Member

A donut could also only be used for mixed states (fail and success). For "everything failed/succeeded" we could still use the x and checkmark. Then you don't have to look too closely if the donut contains any red/green parts.

image

Or maybe a donut with a x or checkmark.


_TBD: Tile design_

Clicking on a list item opens an issueish pane item for the chosen issueish in the same pane container as the parent component (by default, the right dock). If the issuish pane item is already open, it is activated instead.

This comment has been minimized.

@smashwilson

smashwilson Jun 1, 2018

Author Member

Like @simurai and I were talking about earlier, there are a few directions we could go here. To list a few:

  1. Open a "detail view" in a new PaneItem.
    • ...in the same dock as the PR list (how I have it written here)
    • ...in the workspace center (how the PR pane items work today)
    • ...in the workspace center as a pending item
  2. Implement a balloon/tooltip view to host the "details."
  3. Navigate to a "detail view" in-place within the existing PaneItem. Provide some kind of breadcrumb on the detail view to navigate back to the "list view".

This comment has been minimized.

@simurai

simurai Jun 4, 2018

Member

This is super tough to decide.

  1. Something that bothers me with the current FilePatch pane. After committing having to manually close the empty pane. I know that I could just keep it open and it might be re-used, but somehow I always feel the urge to "clean up" and only show what's really needed. Are you also suffering from this disease? The big advantage would be that you're free to move the pane around and maybe even have multiple PR panes open. Even side by side if needed.
  2. A balloon/tooltip is great for quick "in and out", but you can't keep it open to use as a reference because it auto-closes once the focus moves to the buffer.
  3. This is somewhere in between, no clean-up necessary and you can keep it open as long as needed. Downside: It doesn't let you quickly jump between different detail views and you always have to click "back" first. Apple's HIG discourages of using this approach. Plus you're limited to the narrow width of a dock item.

Any favorite? Hmm.... 馃 I don't yet. Maybe best to first mock them up to compare or even do a static prototype.

* "Browse" to launch a browser on the corresponding GitHub page.
* "Merge" to merge the pull request on GitHub if it is open.
* "Close" to close the pull request, unmerged, if it is open.
* "Re-open" to re-open a pull request if it is closed.

This comment has been minimized.

@smashwilson

smashwilson Jun 1, 2018

Author Member

This is a cool opportunity to finally support some of these. I think these actions are what will make those issueish items useful.

This comment has been minimized.

@simurai

simurai Jun 4, 2018

Member

Could it be risky clicking on "Merge" or "Close" without checking the conversation? For example, someone might comment that this PR should use a different approach and isn't ready. I guess that's the reason why on .com these actions are only shown at the end of the conversation, to make sure that you always are aware of new comments.

Maybe not an issue if we would add a "this PR has new comments" notification.

This comment has been minimized.

@smashwilson

smashwilson Jun 6, 2018

Author Member

Yah, really good point. I'm thinking moving these to the bottom is probably a good idea, so you have to pass your eyes over all the PR state on the way to Doing A Thing.

* "Merge" to merge the pull request on GitHub if it is open.
* "Close" to close the pull request, unmerged, if it is open.
* "Re-open" to re-open a pull request if it is closed.
* Pull request overview details: commit count, files changed.

This comment has been minimized.

@smashwilson

smashwilson Jun 1, 2018

Author Member

Future launching-off points to navigable multi-file patch items. Stay tuned for more RFCs 馃幎 馃摶


## Drawbacks

This does not offer a mechanism to create _new_ pull requests, which we have now. We also lose timeline events on the pull request.

This comment has been minimized.

@smashwilson

smashwilson Jun 1, 2018

Author Member

This would actually suck to lose, I use "publish pull request" all the time. @simurai, @donokuda, any thoughts on how we could work this in - preferably without ballooning our scope?

This comment has been minimized.

@simurai

simurai Jun 4, 2018

Member

Couldn't we still show an "Open PR" button where the First list: current pull request is?

### Before RFC merge:

- [ ] How can we still offer "push/publish+new pull request"?
- [ ] What else from the existing issueish pane should we keep? Comments, timeline events?

This comment has been minimized.

@smashwilson

smashwilson Jun 1, 2018

Author Member

The main reason I'd consider taking these out is because they don't really add anything to just... looking at the PR on .com.

This comment has been minimized.

@simurai

simurai Jun 7, 2018

Member

I updated the RFC to keep the existing center pane when clicking on "Conversation". I'm not sure how valuable it is, but since it's already built, we can keep it for now?

Other things from the current GitHub panel:

  • Labels? Might still be ok to keep them?
  • The detailed CI status?
    image
    It takes up quite some space and I'm not sure how valuable it is to see which one is failing. In the current mockup there is the "Checks", but I think that's a new feature that not every repo uses yet. Then the "Able to merge" would include CI, but also if a branch has conflicts or there if still a review is needed. If any of these fail, you would have to click on it and see on .com what it is and take further actions.

This comment has been minimized.

@smashwilson

smashwilson Jun 7, 2018

Author Member

I updated the RFC to keep the existing center pane when clicking on "Conversation". I'm not sure how valuable it is, but since it's already built, we can keep it for now?

馃憤 I like it for:

  • Seeing the full conversation without Massive Endless Cramped Scrolling
  • Detailed checks and build links
  • Being able to drag and drop around as its own thing, so you can open more than one at once - like to keep an eye on one PR to see if it's green while you're working on another one

It takes up quite some space and I'm not sure how valuable it is to see which one is failing.

One of my most-used features, personally 馃榿 It's a great way to keep an eye on builds while continuing to work, and to jump right to the problem when there is one.

Maybe we could come up with a more compact representation (eventually, if not here)?

* Author avatar
* Title
* Controls for actions:
* "Checkout" to fetch (if necessary) and check out the pull request. Only enabled if the current pull request is not the current one.

This comment has been minimized.

@smashwilson

smashwilson Jun 1, 2018

Author Member

"Checkout" for consistent git terminology. Even though it technically fetches first too maybe.

A few edge cases:

  • What if your working copy is dirty? Do we use worktrees? Disable the button? Supporting worktrees properly would be rad, but also pretty complicated.
  • What if you already have the pull request checked out? What if it isn't up to date? Do we automatically pull for you? Maybe only if it works with --ff-only?
* "Close" to close the pull request, unmerged, if it is open.
* "Re-open" to re-open a pull request if it is closed.
* Pull request overview details: commit count, files changed.
* Full description as rendered markdown.

This comment has been minimized.

@simurai

simurai Jun 4, 2018

Member

Since the full description could be pretty long, we could have max height with the option to "show more". Or have the full description at the "end" so you don't have to scroll down too much to see the status checks.

This comment has been minimized.

@smashwilson

smashwilson Jun 6, 2018

Author Member

Could we make the description the bit that scrolls? So something like:

PR #123 - title title
status checks
commit count, files changed

---
description

comments?
---

[merge] [close]

Where the --- denote a scrolling container.

Maybe "Show more" is the better option though. I feel like you're either going to be coming to the PR pane to do something with its summary info (was my build green? has anyone else said anything? how long is this getting? okay, merge this sucker) or to read its description (oh cool @simurai pushed a thing, let's see what he has to say about it) but not both at once. Totally unsubstantiated gut feel though 馃槃

This comment has been minimized.

@simurai

simurai Jun 6, 2018

Member

Could we make the description the bit that scrolls?

If we move the merge/close buttons to the bottom, having the description/comments scrollable, would be a good idea. 馃憤

An alternative to "Show more" for very long descriptions could be a "jump to new comments", in case we wanted to add some sort of read/new notification.


Within the GitHub panel, render a vertical stack of two collapsible lists of _issueish_ (pull request or issue) items:

_First list: current pull request_. If the active branch is associated with one or more open pull requests on the GitHub repository, render an item for each.

This comment has been minimized.

@simurai

simurai Jun 4, 2018

Member

In what case would it show more than one open PR? Is if someone forks use the same branch name and make a PR to your origin? Like suppose you check out a branch called feature on `atom/atom, you might see this?

  • Open PR 1: atom/atom:master <- atom/atom:feature
  • Open PR 2: atom/atom:master <- user1/atom:feature
  • Open PR 3: atom/atom:feature <- user2/atom:feature

image

Couldn't this be reduced to just one if we would limit it to the current remote repo? Then there can only be one branch with the same name.

This comment has been minimized.

@smashwilson

smashwilson Jun 6, 2018

Author Member

Is if someone forks use the same branch name and make a PR to your origin?

Yeah, that's the case where we show multiple now, like all of the "master" PRs it offers to let you pin.

Couldn't this be reduced to just one if we would limit it to the current remote repo? Then there can only be one branch with the same name.

Which remote is the "current" one? Right now we just ask you for the "upstream" one.

This gets tricky when you have a network of multiple forks involved. I think you can send multiple pull requests from the same head ref to different base repositories in the same fork network. You might even be able to open multiple pull requests from the same head ref to different base refs within the same repository... ? I haven't tried it yet.

In any case, the by-far most common case is to have zero or one pull requests for a given head ref and head repository, but we've got a long tail of wacky edge cases where you could have several. I figured that because we're already building a view based on stacked lists, we could handle them and make it more consistent just by making the "current" section a list, too.

This comment has been minimized.

@simurai

simurai Jun 6, 2018

Member

Which remote is the "current" one?

The one that it would use to pull/push. I think it's called "remote tracking branch". But I didn't consider that a local branch could have no remote tracking branch. Hmm.. maybe the title could say

  • "Current PR" if there is just one
  • "Related PRs" if there are more

This comment has been minimized.

@smashwilson

smashwilson Jun 7, 2018

Author Member

Oh, right - I can get that remote from your branch information. Well then 馃槃

* Mini author avatar
* Title, truncated if necessary
* Terse relative timestamp (1d, 2h, 30m)
* Pull request state: open, closed, merged

This comment has been minimized.

@simurai

simurai Jun 5, 2018

Member

If the 2nd list shows only "open" PRs, then we probably don't need to show the state also in each list item. Maybe the title can say "Open PRs" to make it clear. And on the right the repo as a confirmation:

image

This comment has been minimized.

@smashwilson

smashwilson Jun 6, 2018

Author Member

Is there a way to generalize this... ? Like, if every entry in a given list has the same value for some attribute, we omit it from the item display. Or if that attribute is locked to a single value by the query maybe, like if the search is repo:atom/github is:pr is:merged, we could infer that the status is always merged.

This comment has been minimized.

@simurai

simurai Jun 6, 2018

Member

Or if that attribute is locked to a single value by the query maybe

Yeah, that would make it really flexible. And we would need it if we ever wanted to make these lists customizable. Then if the query didn't contain is:pr or is:issue, we would need to show PR or issue icons.

simurai added some commits Jun 5, 2018

* Author avatar
* Title
* Controls for actions:
* "Checkout" to fetch (if necessary) and check out the pull request. Only enabled if the current pull request is not the current one.

This comment has been minimized.

@Arcanemagus

Arcanemagus Jun 6, 2018

Only enabled if the current pull request is not the current one.

This sentence is recursive. Did you mean something like this?

Only enabled if the current checked out branch is not the branch of the pull request.

If so I would go with "visible" myself instead of enabled, but if you don't want the UI "jumping" on users enabled is better.

This comment has been minimized.

@smashwilson

smashwilson Jun 7, 2018

Author Member

If so I would go with "visible" myself instead of enabled, but if you don't want the UI "jumping" on users enabled is better.

Yeah, my general preference is to avoid hiding temporarily inapplicable controls - if you don't see a button, you have no idea you could check out a PR, but if the button is there and disabled, you know that you can check out some PRs but not this one. (You can even add an explanation why on the tooltip).

Not a hard-and-fast rule, though. For example, we should only show "Reopen" on a closed PR, because we'll always be able to show some kind of "PR action button cluster" in the same place and that's enough of a cue for that functionality.

Also thanks for catching my tangled wording there 馃憤 馃檱

* `Checks` with count, links to .com (for now)
* `Files changed` with count, links to .com (for now)
* Plus a "Checkout" button to fetch (if necessary) and check out the pull request. Only enabled if the current pull request is not the current one.
* `Conversation` with comment count.

This comment has been minimized.

@simurai

simurai Jun 6, 2018

Member

When making the mockup I had a deja-vu 馃槃 and sure enough, there is a closed PR that included the conversation and comments in a panel (now dock item): #494

It was ok for small PRs. But for PRs that had a long description and lots of long comments, it just felt too cramped and you scroll forever. Here a gif that tries to simulate it with this PR: #1370

timeline

We then concluded that it's better to open a center pane for the conversation. So not sure if we should try again having the conversation in a dock item? An alternative might be to only have the description, but not the comments. If you want the read the whole timeline with comments, click on Conversation and it links to .com.

This comment has been minimized.

@smashwilson

smashwilson Jun 7, 2018

Author Member

Hahaha! Oops.

We then concluded that it's better to open a center pane for the conversation.

馃憤

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jun 7, 2018

Here a slightly different proposal. I'll make first a comment before editing. Note: The "Reviews" are already included just to show how they could be integrated, but they won't be part of this RFC.

1. Preview popover

In order to quickly get a preview with more details about a PR, when clicking on a "Open PRs" list item, a popover shows up:

popover

2. Checkout

Clicking on the "Checkout" button...

image

...checks out that branch. Also the popover closes and the PR list changes to the "Current PR":

prs 4

It's similar to the popover, but it's more permanent and doesn't disappear when focus moves away. Clicking on "< PRs" at the top, switches back to the PR lists again.

3. Conversation

From the popover or "Current PR" sub view, you can click on "Conversation" which opens a center pane with the PR description and timeline that is currently already implemented.

prs

prs 3

Clicking on the rest (Commits, Checks, Files changed) opens the page on .com for now. In the future they might also open a center pane.

image

Too fancy?

Having the popover AND the "Current PR" sub view would be nice, but it also makes the implementation more complex. Maybe we first should create a simple version to validate if people even find a list of "Open PRs" useful. We can always iterate on it in a next phase.

Remove comments
And keep the timeline center pane.
@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jun 7, 2018

I am a fan of the "popover with a jump to a full pane" approach - kind of a blend of options (1) and (2) from up above. It's also similar to the way we wrote up the commit detail popup. The only downside is that it isn't a familiar Atom navigation mechanism elsewhere, but I think that's okay.

I am 馃憥 on the subview though:

  • There's no way to get to a similar view of non-current PRs, which feels like an odd special case;
  • The information it presents is largely the same as the popup;
  • It's mutually exclusive with the PR list - there's no way to see both at once;
  • It isn't obvious how to get back to it after you return to the PR list, so it introduces asymmetric navigation which always makes me wary.

How would you feel about adding an "open" button to the popup balloon that opens the "full pane item" view of that PR, with the full conversation and the other stuff that's visible in the popup (commit count, status checks, etc)? Maybe we could open it on a double-click on the balloon as well. That would kind of mirror the "pending 馃憠 permanent" pane transition, but still give you mechanisms for both a quick glance at a PR's summary and a deep dive into its full state that you can drag and drop around, open multiple of, and treat in familiar "pane item" ways.

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jun 7, 2018

FYI: My plans are to merge this RFC and begin initial work early next week, after I get back from a weekend trip. That doesn't mean we can't change our minds later, I just want to draw a line and start somewhere 馃槅

simurai added some commits Jun 7, 2018

Add PR number to list item
Some teams use these numbers a lot when talking about PRs.
@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jun 8, 2018

How would you feel about adding an "open" button to the popup balloon that opens the "full pane item" view of that PR, with the full conversation and the other stuff that's visible in the popup (commit count, status checks, etc)?

Yeah, I was thinking the same thing. But not sure.. 馃槃 I was more thinking we can keep the current conversation center pane, since it's already built, but not invest more time on it by adding the popover content. Because I wonder how many people actually use it vs. just open the PR page on .com that is fully featured.

What about this:

  1. Phase: Make list items open as new dock items (like it currently is proposed).
  2. Phase: Consider adding a popover and maybe adding the popover content also to the center pane.

With a dock item, you could split it to the left and kinda simulate like it would be part of the center pane.

split 2


* Mini author avatar
* Title, truncated if necessary
* PR number (`#1503`)

This comment has been minimized.

@simurai

simurai Jun 8, 2018

Member

Apparently in some teams, they use the PR number/ID to refer to a PR. Therefore it should be helpful seeing it in the list so you can search by number/ID and not just title.

* 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](https://github.com/atom/github/pull/1503#partial-pull-merging)
* "Merge PR" to merge the pull request on GitHub if it is open.

This comment has been minimized.

@simurai

simurai Jun 8, 2018

Member

In our design review, it was suggested to not show the close and merge button. Just because there isn't enough information and context to make a good decision. It might still be handy for people that mostly merge their own PRs, but it could also lead to "accidents".

### Accordion Lists

Within the GitHub panel, render a vertical stack of two collapsible lists of _issueish_ (pull request or issue) items:

This comment has been minimized.

@annthurium

annthurium Jun 8, 2018

Contributor

reading this, it wasn't immediately clear to me that the number was "time." I saw "3M" and thought "3 million"? So can we clarify that label somehow? Also is this the date that the pull request was opened, or the date of the first commit in that pull request?

This comment has been minimized.

@Arcanemagus

Arcanemagus Jun 8, 2018

I thought the same thing, and was going to comment on it before realizing it is using M to signify months since m would be taken by minutes.

This comment has been minimized.

@annthurium

annthurium Jun 8, 2018

Contributor

yeah, ideally we could use "minutes", "weeks", "etc" instead of abbreviations. Moment.js has a "humanize time since" function that provides this in an i18n-friendly manner. https://momentjs.com/docs/#/durations/humanize/

This comment has been minimized.

@simurai

simurai Jun 11, 2018

Member

For the recent commits we changed the longer 10 minutes ago to the shorter 10m so that there is more space for the commit message. It's similar Twitter's short version.

To avoid confusion, an option might be to never show minutes and only start with hours, like

  • Now or <1h
  • 1h
  • 1d
  • 1m
  • 1y

That might still be fine not knowing the exact minutes.


## Implementation phases

1. Accordion list infrastructure: search model, collapsible list component.

This comment has been minimized.

@annthurium

annthurium Jun 8, 2018

Contributor

are you going to make a generic "list" component that could be re-used elsewhere in the UI? It seems like we have a lot of "one off" components, and it might make sense to think about what could be made generic. (Ideally, I'd love a React UI components library for Atom that provides generic modals and lists and forms and other stuff, but one thing at a time.)

This comment has been minimized.

@smashwilson

smashwilson Jun 9, 2018

Author Member

Are you going to make a generic "list" component that could be re-used elsewhere in the UI?

Yah, I was mentally planning on having an AccordionList component.

It seems like we have a lot of "one off" components, and it might make sense to think about what could be made generic.

This is true, we don't as of yet have many reusable building block-style components. Does anything come to mind for abstractions we could introduce to consolidate existing ones? That would be a cool way to improve the codebase 馃

Ideally, I'd love a React UI components library for Atom that provides generic modals and lists and forms and other stuff, but one thing at a time.

I've had Secret Plans 鈩笍 to extract our Atom API integration components to their own npm package for a while. We could build on that 馃槃

This comment has been minimized.

@simurai

simurai Jun 11, 2018

Member

馃憤 I would also love if Atom would provide UI components beyond the basic ones from the styleguide. Then not every package author has to create their own from scratch.

having an AccordionList component

A while (very long time) ago, I made an accordion component: atom/atom-ui#14

accordion

It uses the <details> and <summary> HTML elements to provide the collapsing functionality. Not sure if we wanted to use that. Or just initially?

This comment has been minimized.

@annthurium

annthurium Jun 11, 2018

Contributor

yeah, if we care about Atom's "hackability" providing a generic UI toolkit that package authors could leverage seems like a REALLY high value thing. (And then we can also bake in accessibility / i18n). But one thing at a time, since creating a UI toolkit is a large undertaking.

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jun 11, 2018

Taking a step back, can you imagine using this proposed PR list? I can think of these scenarios where a PR list would be handy:

  • See the CI status of a PR even when on a different branch.
  • Switch/checkout a branch based on the PR title or PR number. For example if someone says: "I pushed it to [link]" in Slack. Since you can see the PR number in the URL, you can open Atom directly, find the PR and checkout the branch.

And it's meant "As an initial building block toward a pull request review workflow", but I'm not quite sure if I would use the PR list as an entry point when doing a PR review. In most cases, I probably first end up on the PR page on .com and just then decide to review it. The reason is that when getting a notification about a new PR, I either save it to my personal TODO list or just keep the browser tab open. Then whenever I have time, I'll come back to do the review. Other people might have different places where they keep these reminders, like email inbox, project board, but they all have in common that you will click on a link and end up on the PR page on .com. Which brings me to:

It might be worth to make the "I'm looking at a line on .com's diff, I want that same line being open in Atom" as quick and easy as possible? I'll make a separate RFC and we can discuss it there.

ps. I don't mean to say we shouldn't start implementing this RFC, it would be a big improvement over the current version. Just that it would still be a very long way to "I never have to leave Atom" and in the meantime there is a chance to greatly improve the .com <-> Atom workflow.

smashwilson added some commits Jun 11, 2018

@smashwilson smashwilson merged commit 720e112 into master Jun 11, 2018

3 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

@smashwilson smashwilson deleted the aw/rfc-pr-list branch Jun 11, 2018

@simurai simurai referenced this pull request Jun 18, 2018

Merged

Pull Request list #1523

20 of 20 tasks complete

@simurai simurai referenced this pull request Jul 20, 2018

Merged

Restyle hovercards #1593

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.