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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recent commit changes RFC #1318

Merged
merged 14 commits into from Mar 28, 2018

Conversation

Projects
None yet
4 participants
@smashwilson
Member

smashwilson commented Feb 22, 2018

A more formal proposal of the "recent commits" view displayed beneath the mini commit editor.

Previous discussion and design ideating are available in #554.

Acceptance criteria

We will merge this RFC and begin work when we accumulate reviews from @smashwilson, @kuychaco, and @simurai.

Rendered

Implementation phases

  • Convert GitTabController and GitTabView to React. #1319
  • List read-only commit information. #1322
  • Replace the amend checkbox with the "undo" control. #1328
  • Context menu with actions. #1364
  • Show which commits have been pushed.
  • Balloon with action buttons and additional information.

smashwilson added some commits Feb 22, 2018

@smashwilson smashwilson added the rfc label Feb 22, 2018

smashwilson added some commits Feb 22, 2018

If the active repository has no commits yet, display a short panel with a background message: "Make your first commit".
Otherwise, display a **recent commits** section containing a sequence of horizontal bars for each of the top three commits reachable from the current `HEAD`, with the most recently created commit on top. The user can resize the recent commits section. As it is expanded or shrunk, the number of visible commits is changed responsively.

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

After creating new branch, only display commit that was branched off of. This may be technically challenging to implement (how do we know when to do this? last reflog entry is a checkout?).

This comment has been minimized.

@smashwilson

smashwilson Feb 23, 2018

Member

Oh hey there's a StackOverflow question about this:

git log mybranch --not $(git for-each-ref --format='%(refname)' refs/heads/ | grep -v "refs/heads/mybranch")
On the most recent commit, display an "undo" button. Clicking "undo" performs a `git reset` and re-populates the commit message editor with the existing message.
If any of the recent commits have been pushed to a remote, display a divider after the most recently pushed commit that shows an octocat icon. On hover, show the name of the remote tracking branch.

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

Additionally add dividers for other refs (branches and tags)

2. List read-only commit information.
3. Replace the amend checkbox with the "undo" control.
4. Context menu with actions.
5. Tooltip with action buttons and additional information.

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member
  • Show which commits have been pushed
  • Surface info about other refs
If the active repository has no commits yet, display a short panel with a background message: "Make your first commit".
Otherwise, display a **recent commits** section containing a sequence of horizontal bars for each of the top three commits reachable from the current `HEAD`, with the most recently created commit on top. The user can resize the recent commits section. As it is expanded or shrunk, the number of visible commits is changed responsively.

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

Just show last three commits? Or allow seeing more via scrolling or dragging to make view larger?

This comment has been minimized.

@smashwilson

smashwilson Feb 28, 2018

Member

Thanks to @simurai magic scrolling is a go. If we can, I'd really like to make it resizable by dragging too. If it's too technically challenging we can drop it, just like the "only show commits reachable by the current HEAD" bit up above, but I want to leave it in the RFC as a behavior goal if that's okay. I think I just feel like it'd be too frustrating not to be able to do 😁

If the active repository has no commits yet, display a short panel with a background message: "Make your first commit".
Otherwise, display a **recent commits** section containing a sequence of horizontal bars for each of the top three commits reachable from the current `HEAD`, with the most recently created commit on top. The user can resize the recent commits section. As it is expanded or shrunk, the number of visible commits is changed responsively.

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

Also show commits that are on remote branch but not local (as a result of auto-fetch). Punt to show in full-featured log and just link to that? Accordion style (expand/collapse). @simurai make mock-up to feel it out

On the most recent commit, display an "undo" button. Clicking "undo" performs a `git reset` and re-populates the commit message editor with the existing message.
If any of the recent commits have been pushed to a remote, display a divider after the most recently pushed commit that shows an octocat icon. On hover, show the name of the remote tracking branch.

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

@simurai would you mind making some quick mockups showing the dividers for remote tracking branch, local branch, tag. Also show icon and ref name visible (ex ref names: origin/feature-branch, master, v1.0.0) 🙏

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

consider when there are multiple refs for a given commit

* For the most recent commit only, an "Amend" option. "Amend" is enabled if changes have been staged or the commit message mini-editor contains text. Choosing this applies the staged changes and modified commit message to the most recent commit, in a direct analogue to using `git commit --amend` from the command line.
* A "Revert" option. Choosing this performs a `git revert` on the chosen commit.
* A "Hard reset" option. Choosing this performs a `git reset --hard` which moves `HEAD` and the working copy to the chosen commit. When chosen, display a modal explaining that this action will discard commits and unstaged working directory context. Extra security: If there are unstaged working directory contents, artificially perform a dangling commit, disabling GPG if configured, before enacting the reset. This will record the dangling commit in the reflog for `HEAD` but not the branch itself.
* A "Soft reset" option. Choosing this performs a `git reset --soft` which moves `HEAD` to the chosen commit and populates the staged changes list with all of the cumulative changes from all commits between the chosen one and the previous `HEAD`.

This comment has been minimized.

@smashwilson

smashwilson Feb 23, 2018

Member

Plus: "mixed reset," consistent with the tool-tip actions 🎶

* A "Hard reset" option. Choosing this performs a `git reset --hard` which moves `HEAD` and the working copy to the chosen commit. When chosen, display a modal explaining that this action will discard commits and unstaged working directory context. Extra security: If there are unstaged working directory contents, artificially perform a dangling commit, disabling GPG if configured, before enacting the reset. This will record the dangling commit in the reflog for `HEAD` but not the branch itself.
* A "Soft reset" option. Choosing this performs a `git reset --soft` which moves `HEAD` to the chosen commit and populates the staged changes list with all of the cumulative changes from all commits between the chosen one and the previous `HEAD`.
On click, reveal a tool-tip containing:

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

In the future when we have a multi-file diff view, we can consider just opening a pane item with this information

- Providing a bridge to navigate to an expanded log view that allows more flexible and powerful history exploration.
- Show an info icon and provide introductory information when no commits exist yet.
- Add a "view diff from this commit" option to the recent commit context menu.
- Integration with and navigation to "git log" or "git show" pane items when they exist.

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

How do we surface that we've made a commit for you before doing a git reset --hard? So you never actually lose any unstaged changes

This comment has been minimized.

@kuychaco

kuychaco Feb 23, 2018

Member

Right-click context menu on history? "Undo reset" until next commit is made.

2. List read-only commit information.
3. Replace the amend checkbox with the "undo" control.
4. Context menu with actions.
5. Tooltip with action buttons and additional information.

This comment has been minimized.

@smashwilson

smashwilson Feb 23, 2018

Member

"Balloon" instead of "tooltip" 🎈

@simurai

This comment has been minimized.

Member

simurai commented Feb 27, 2018

Here a few dividers:

With label

divider1

Icon only

divider2

Tag

divider3

Hmm.. not sure. They use quite some space for such a small area and it's not clear if they are referring to the commits above or below a divider. Might fit better for the more fully-featured history log panel.

Alternatively we could have up/down arrows on the commits, matching the ahead/behind in the status-bar:

arrows

And tags could also be attached to a commit:

tag

@smashwilson

This comment has been minimized.

Member

smashwilson commented Feb 28, 2018

Ohh, fancy

Looking over the options, some thoughts:

  • I'd agree that the octocat divider and the dedicated tag line both occupy a little too much vertical real estate for the information they give.

  • The up and down arrows are interesting! I like that they preserve the context of which commits are only local and only remote even if when the remote tracking branch ref isn't visible, and the symmetry with the push and pull buttons.

  • I really like the bubble-ref in your last .gif - in fact, I think I prefer that to any of the other full-line dividers. We do need to be sure that it scales correctly when many refs refer to a single commit. Maybe after we reach some low ref threshold (two?) they get their own horizontal row above the message? Something like this:

    image

@smashwilson

This comment has been minimized.

Member

smashwilson commented Feb 28, 2018

I've updated the writeup based on the review comments. @kuychaco, @simurai, care to have another look and review when you get the chance? 🙇

@ampinsk

This comment has been minimized.

ampinsk commented Feb 28, 2018

Hey all! @donokuda asked me to take a look here because of the overlap in problem space with some things we're working on at GHD.

Taking a look at the options you have here, I really like the icons direction. I think it does the best job working within the small timeline here. But, I think there might be a clearer icon to use than the up/down arrow. To me at least, the up/down arrow is pretty strongly associated with the push/pull action, not necessarily with the commit itself in terms of "this commit is ahead of my branch"

It would be awesome to come up with a new icon that we can use (here and in GHD!) to signal "this commit is ahead/behind your branch" Maybe something like |→ and ←| ?

Hope this is helpful! Let me know if I can clarify anything :)

@simurai

This comment has been minimized.

Member

simurai commented Mar 1, 2018

@smashwilson We do need to be sure that it scales correctly when many refs refer to a single commit.

Here a few refs below and aligned with the message:

refs

If they would be above, it might look like they belong to the commit above. We could also add divider lines for each commit, but then it quickly looks more heavy.

Hmm... also I think having these refs on a 2nd line makes messages less scannable because you get interrupted with something different when scrolling. How important is it to see these refs in the recent commits? I can see two options:

  1. Only show tags (in the same line after the message). Their length (no more than vX.X.X) is somewhat predictable.
  2. Show other refs on a 2nd line, but then disable scrolling.
@simurai

This comment has been minimized.

Member

simurai commented Mar 1, 2018

@ampinsk Maybe something like |→ and ←| ?

👍 Yeah, a hint of their meaning could come from how branches show the ahead/behind:

image

artboard2

The up/down arrows would be more from a local/remote perspective. Like this cheesy way of thinking that the remote is somewhere up in the cloud. Kinda like downloading and uploading.

commit ⬇︎ (you're behind: pull this down)
---- remote / local -----
commit ⬆︎ (you're ahead: push this up)
commit ⬆︎ (you're ahead: push this up)
commit

Looking at the up/down arrows, people might try to click them to actually push/pull? 🤔 Here with clouds around the arrows:

artboard

simurai added some commits Mar 1, 2018

3. Replace the amend checkbox with the "undo" control.
4. Context menu with actions.
5. Balloon with action buttons and additional information.
6. Show which commits have been pushed.

This comment has been minimized.

@simurai

simurai Mar 1, 2018

Member

I think it should say have been "can be" pushed. And also show commits that can be pulled (after fetching).

And also move it up in the priority list, maybe as far as 3rd place?

@simurai

This comment has been minimized.

Member

simurai commented Mar 2, 2018

Here more feedback from @ampinsk + @donokuda. ❤️

  1. When reloading Atom, it shows the "Make your first commit" for 1-2 seconds before showing the recent commits.
  2. An alternative to the ahead/behind icons -> Have greyed out/bold text, different background color, or some other kind of highlight
  3. Controversial: Instead of showing avatars -> show commit icon, with a vertical line like .com. This allows to have different states or icons like ahead/behind.
  4. Don't show refs! Takes up too much space makes it harder to scan the messages.

And here the 2nd suggestion visualized:

screen shot 2018-03-02 at 5 36 11 pm

@kuychaco

This comment has been minimized.

Member

kuychaco commented Mar 28, 2018

We've completed Phases 1 and 2 with Phase 3 almost complete. With that we've reached our near-term goals for this feature 🎉

The rest is icing and we'll hold off until future quarters in order to prioritize progress on other roadmap items. I'll go ahead and merge this now so that we can save all of the goodness to refer to when we pick this back up again.

Thanks everyone for your fabulous input!

@kuychaco kuychaco merged commit fea70d2 into master Mar 28, 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

@kuychaco kuychaco deleted the rfc-recent-commits branch Mar 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment