Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

RFC for Issueish List View #1503

Merged
merged 18 commits into from
Jun 11, 2018
Merged

RFC for Issueish List View #1503

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e655ac1
Draft draft draft
smashwilson Jun 1, 2018
eecd177
Add tile mockup
simurai Jun 5, 2018
45b116b
Add lists mockup
simurai Jun 5, 2018
8a9a17f
Add pane mockup
simurai Jun 5, 2018
0e84984
Update pane title
simurai Jun 5, 2018
db263ce
Update PR detail pane
simurai Jun 6, 2018
8ab7761
Remove comments
simurai Jun 7, 2018
9f7323c
Add new PR option
simurai Jun 7, 2018
55de6f9
:memo: Fix typo
simurai Jun 8, 2018
20ed5c6
Add PR number to list item
simurai Jun 8, 2018
c5e174e
Add CI status to PR pane
simurai Jun 8, 2018
8070406
Specify what constitutes the "current" PR
smashwilson Jun 11, 2018
be873aa
Specify the PR list order
smashwilson Jun 11, 2018
766c5fd
We're starting with a regular pane item for issueishes
smashwilson Jun 11, 2018
77e63ba
Some drawbacks
smashwilson Jun 11, 2018
df128dd
Questions are answered, yay
smashwilson Jun 11, 2018
728ac17
Move "merge"/"close"/"re-open" actions out of scope
smashwilson Jun 11, 2018
f971b7d
Accept the RFC as 002
smashwilson Jun 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 98 additions & 0 deletions docs/rfcs/004-issueish-list.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
# Issueish List

## Status

Proposed

## Summary

Display a list of all open pull requests in the current repository in the GitHub tab.

## Motivation

To provide a navigational element that makes sense even if you aren't on an active feature branch.

To give users a way to see an overview of what's going on in the repository.

As an initial building block toward a pull request review workflow.

## Explanation

### Accordion Lists

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_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.
Copy link
Contributor

@simurai simurai Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


_Second list: all open pull requests_. List all open pull requests on the GitHub repository.

Each list has a "collapse arrow" in its header. Clicking the collapse arrow toggles the visibility of that list's items, accordion-style.

If either list exceeds 20 items, truncate the list and render a "More" link after its final item. Clicking "more" opens the corresponding search on GitHub.

Each list item renders a tile containing a compact set of information about that pull request:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* Status check summary
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ 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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


### Issueish Pane Item: Pull Request

For a pull request, the issueish pane shows:

* 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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?

Copy link

@Arcanemagus Arcanemagus Jun 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍 🙇

* "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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* Pull request overview details: commit count, files changed.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future launching-off points to navigable multi-file patch items. Stay tuned for more RFCs 🎶 📻

* Full description as rendered markdown.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* Reaction emoji and counts.
* Details and links for all status checks.

_TBD: Issueish pane item design_

## 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@simurai simurai Jun 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


## Rationale and alternatives

Our current GitHub panel focuses on showing you stuff about _the pull request that's associated with your current branch._ The problem is, it's difficult to unambiguously determine that in the general case.

The first thing you see today when you open the GitHub panel on the `master` branch of an active repository is not very helpful:

![wat](https://user-images.githubusercontent.com/17565/40857603-99b92304-65a9-11e8-986e-0f14290bda8a.png)

This is a list of _all pull requests on GitHub that have a head ref called "master", from any head repository_. You can then "pin" any of them to see that pull request's details. This isn't useful on master and it's unclear to users what this is supposed to accomplish. Pinning was intended to be an infrequent edge case when we couldn't find the right PR for a given ref, not the first interaction you have with the package.

Showing a PR list instead provides a uniform, more easily understood entry point to the package's GitHub functionality, and paves the way to other pull-request-focused activities in the future. "All open PRs" seems like a reasonable starting point, and "current PRs" preserves the ability to take advantage of your local editor context.

With that said, the choices for the specific lists we show are a bit arbitrary. We'll need to research and iterate on them quite a bit to find what's most useful for the most people, but for now we need to start with something.

## Unresolved questions

### 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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

- [ ] Are there other pull request actions it would be useful to support?

### Out of scope:

- [ ] How can we allow a user to customize the lists?
- [ ] How can we notify a user about updated activity on a visible PR?

## Implementation phases

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2. Revisit the issueish pane item and add action controls.