The Wayback Machine - https://web.archive.org/web/20190429042538/https://github.com/atom/github/pull/1807
Skip to content

/github

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

View commit details in pane #1807

Merged
merged 220 commits into from Dec 11, 2018

Conversation

Labels
None yet
6 participants
@kuychaco @coveralls @smashwilson @annthurium @simurai @vanessayuenn
@kuychaco
Copy link
Member

commented Nov 22, 2018 •
edited by smashwilson

Description of the Change

Addresses #1655 to add pane to view the contents of a single commit.

Remaining work

  • handle case when commit does not exist in repo
  • fill out this template!
  • remove jump to file
  • try making highlighting more subtle (in Commit Item only) @simurai
  • try making commit message fixed (time box)
    • make commit message body collapsable
  • fix problem where diffs aren't scrollable / fixed height issue
  • add UI entry points from the commits tab on the GitHub pull request pane (only when PR is checked out)
    • make commit message appear clickable when PR is checked out
  • add UI entry point in timeline view
  • make background color for recent commit different upon hover @simurai
  • keyboard navigation in recent commits view
    • clicking commit opens CommitItem but retains focus on recent commit view
    • up/down arrows for navigation
    • enter to open CommitItem
    • cmd-left to open and focus CommitItem
    • cmd-right from CommitItem to focus commit entry in RecentCommitView
    • tab and shift-tab should always advance and retreat focus from the CommitView editor
    • tab and shift-tab should move focus between the commit button and the RecentCommitView
  • open CommitDetailItem by clicking on commit in recent commit history
    • selected state and focused state for CommitDetailItem
  • remove functionalities from file patch header and hunk header that don't make sense for viewing commit details
  • make commit message selectable for copy&paste
  • link to dotcom commit view somehow
    • don't show link if repo isn't hosted on github
    • don't show link if commit has not been pushed
  • fix date rendering
  • remove staging and unstaging buttons from file mode and symlink changes.
  • fix TODOs in code
  • an open commit detail item should close when we undo the corresponding commit
  • open commit detail pane as pending pane
  • use the same logic we use to determine if a PR is able to be checked out, to conditionally style the commits in that pull request so users know they're clickable.
    • related bug: clicking a commit in timeline view that is not clickable (doesn't exist in active repo) still calls openCommit and opens a commit details pane item with an endless spinner. we have CSS that makes it appear unclickable, but the click handler is registered regardless of clickability...
    • clickable commit message headers in a checked-out PR need CSS work
  • when clicking commit in recent commit history, maintain focus in recent commit history. If you click on a commit that isn't open in a pane, the new pane items steals the focus.

Alternate Designs

  • We went back and forth about how to handle the edge case with a loonnnngggg commit message body. Most commits will not have one, but if somebody did write a novel in there it's probably really important. Dotcom just shows you the entire commit message body (which is kinda ugly) and Desktop sets a max height on the commit message, so that you can see the entire message and then check out the actual diff beneath it. The implementation we went with is slightly more complicated but hopefully a better experience. We compute whether it's a long diff. If so, truncate the commit message body and display a "show more" button. There is still a max height on the commit message, so both the message and the code beneath remain scrollable.

Benefits

  • Users will be able to more easily perform code archaeology in the comfort of their editor
  • Since the RecentCommitsView highlights on hover now, the context menu may be more discoverable. (during user research for Commit Preview, it came up that having highlighting here might make the context menu more discoverable.)
  • We made some improvements to our focus management code along the way, so that you can now tab navigate from the commit input box to the RecentCommitsView. (And also hopefully made the code clearer.)

Possible Drawbacks

  • More code, more problems? I don't know.

Applicable Issues

#1655

Metrics

  • Count the number of opened commit detail items.
  • Count entry points individually: from the command palette command, from the RecentCommitsView, and from the pull request detail item.

These can help us ensure that the CommitDetailItem is discoverable and useful for users.

Test Coverage

  • no uncovered new lines on Coveralls
  • no uncovered existing lines on Coveralls

Manual verification

  • in the RecentCommitsView:
    • users can click on recent commits and open a commit pane
    • users can navigate the recent commits pane with a keyboard and focus is maintained
  • in the pull request pane:
    • On the "Commits" tab:
      • if the commit exists in the repo, the commit is clickable and opens a pane
      • if the commit does not exist in the repo, it's unclickable
    • On the commits in the timeline view:
      • if the commit exists in the repo, the commit is clickable and opens a pane
      • if the commit does not exist in the repo, it's unclickable
  • in the commit pane item:
    • pushed commits include a link to dotcom
    • unpushed commits do not include a link to dotcom
    • ensure that merge commits display as expected
    • commits with symlink changes should not have staging/unstaging buttons
    • commits with filemode changes should not have staging/unstaging buttons
    • when attempting to open a commit sha that doesn't exist in repo, we show the user an error message
    • when attempting to open a commit that isn't a valid sha, we show the user an error message.
    • if you undo a commit that's open in a pane, don't close the commit pane.
    • ensure that methods handle empty repo edge case
    • ensure that methods handle case where repo has only one commit
    • lllooonnnggg commit message bodies still allow users to scroll the file patch item below when expanded
    • long commit message bodies are truncated, and collapse / expand as expected

Documentation

Release Notes

The GitHub package now has a pane item that displays the contents of an individual commit.

User Experience Research (Optional)

It might be useful to do research around:

  • discoverability. Do people find the entry points for this feature?
  • Does making the RecentCommitsView highlight the action item actually make context menus more discoverable? (This is something that came up during UXR for the Commit Preview feature, that needs more investigation.)
  • When users do discover the feature, do they find it useful? Is the UI what they expect?

Follow-on work

Optional tasks we can tackle if time permits. Otherwise, we can file them as follow-on issues.

  • Convert CommitPreviewItem to CommitItem when commit action is completed.
  • Display github handles instead of email addresses.
  • Display parent commits.
    • Clicking commit sha will open another CommitDetailItem.
  • Add prev/next arrows to navigate across git history.
@coveralls

This comment has been minimized.

Sign in to view
Copy link

commented Nov 22, 2018 •
edited

Coverage Status

Coverage increased (+1.3%) to 86.807% when pulling 72b38bb on ku-commit-item into 1a1a315 on master.

Tilde Ann Thurium and others added some commits Dec 6, 2018

Tilde Ann Thurium
rename `item` to `commit` as per Katrina's suggestion
Loading status checks…
25a5a44
Tilde Ann Thurium
fix commit-view tests
94b60a3
Tilde Ann Thurium
clean up commit-view tests a bit 
make them use a builder function so they're less repeititive
clean up prop type errors in console
Loading status checks…
5196c83
Tilde Ann Thurium
👕 love when my in-editor linting stops working mysteriously
Loading status checks…
1c67319
@simurai
Style commit buttons
Loading status checks…
9f7b337
@smashwilson
No need to have those props optional right now
Verified
This commit was signed with a verified signature.
@smashwilson smashwilson Ash Wilson
GPG key ID: 81B1DDB704F69D2A Learn about signing commits
Loading status checks…
4dda18c
@smashwilson
Leave CommitDetailItems open during undo
Verified
This commit was signed with a verified signature.
@smashwilson smashwilson Ash Wilson
GPG key ID: 81B1DDB704F69D2A Learn about signing commits
Loading status checks…
63747af

@smashwilson smashwilson referenced this pull request Dec 7, 2018

Merged

Add a quick blurb about viewing commit details #505

@smashwilson

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 7, 2018

Demo .gif:

commit-detail-item
@smashwilson
Forgot a word in there
Verified
This commit was signed with a verified signature.
@smashwilson smashwilson Ash Wilson
GPG key ID: 81B1DDB704F69D2A Learn about signing commits
Loading status checks…
65222d6
@kuychaco

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Dec 8, 2018

Love the gif! Would it be worth showing off the feature to toggle visibility of the commit message description when it's long? I don't have strong feelings one way or another, just thinking that it could showcase more of the value of viewing a commit in the pane (more context from the commit message)
@kuychaco

kuychaco reviewed Dec 8, 2018
View changes

Copy link
Member Author

left a comment

This PR is looking great! I've finished reviewing all of the files in lib. Not gonna sweat too much about reviewing the tests. Everything looks solid, though I left a couple of comments for minor logic improvements for good measure.
Show resolved Hide resolved lib/git-shell-out-strategy.js
Show resolved Hide resolved lib/controllers/recent-commits-controller.js
Show resolved Hide resolved lib/views/open-commit-dialog.js Outdated
Show resolved Hide resolved lib/models/commit.js Outdated
Show resolved Hide resolved lib/models/commit.js Outdated
lib/views/timeline-items/commit-view.js
/>
<a className="commit-sha" href={commit.commitUrl}>{commit.oid.slice(0, 8)}</a>
<p className="commit-message-headline">
{this.props.onBranch

This comment has been minimized.

Sign in to view
Copy link
@kuychaco

kuychaco Dec 8, 2018

Author Member

💡 Out of scope for this PR, but it would be cool to offer the ability to view commits that exist in the repo but aren't on the checked out branch. So, for example, if I want to cherry-pick a commit I can just view the commit contents to verify it is what I think it is without having to checkout the branch that the commit is on.

How much more complexity would this introduce? We'll have to handle the edge case where the user hasn't pulled down commits yet and either prompt them to pull or do a pull for them... and probably other things to consider...

kuychaco and others added some commits Dec 11, 2018

@kuychaco
🔥 unnecessary `autoHeight={false}` 
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Don't forget the following emoji:
#
# * 🎨 `🎨` when improving the format/structure of the code
# * 🐎 `🐎` when improving performance
# * 🚱 `🚱` when plugging memory leaks
# * 📝 `📝` when writing docs
# * 🐧 `🐧` when fixing something on Linux
# * 🍎 `🍎` when fixing something on Mac OS
# * 🏁 `🏁` when fixing something on Windows
# * 🐛 `🐛` when fixing a bug
# * 🔥 `🔥` when removing code or files
# * 💚 `💚` when fixing the CI build
# * ✅ `✅` when adding tests
# * 🔒 `🔒` when dealing with security
# * ⬆️ `⬆️` when upgrading dependencies
# * ⬇️ `⬇️` when downgrading dependencies
# * 👕 `👕` when removing linter warnings
e2cfa62
@kuychaco
🎨 pass `isValidEntry` prop to OpenCommitDialog rather than repository 
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Don't forget the following emoji:
#
# * 🎨 `🎨` when improving the format/structure of the code
# * 🐎 `🐎` when improving performance
# * 🚱 `🚱` when plugging memory leaks
# * 📝 `📝` when writing docs
# * 🐧 `🐧` when fixing something on Linux
# * 🍎 `🍎` when fixing something on Mac OS
# * 🏁 `🏁` when fixing something on Windows
# * 🐛 `🐛` when fixing a bug
# * 🔥 `🔥` when removing code or files
# * 💚 `💚` when fixing the CI build
# * ✅ `✅` when adding tests
# * 🔒 `🔒` when dealing with security
# * ⬆️ `⬆️` when upgrading dependencies
# * ⬇️ `⬇️` when downgrading dependencies
# * 👕 `👕` when removing linter warnings
140bc97
@kuychaco
Fix OpenCommitDialog tests (use `isValidEntry` rather than repository) 
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Don't forget the following emoji:
#
# * 🎨 `🎨` when improving the format/structure of the code
# * 🐎 `🐎` when improving performance
# * 🚱 `🚱` when plugging memory leaks
# * 📝 `📝` when writing docs
# * 🐧 `🐧` when fixing something on Linux
# * 🍎 `🍎` when fixing something on Mac OS
# * 🏁 `🏁` when fixing something on Windows
# * 🐛 `🐛` when fixing a bug
# * 🔥 `🔥` when removing code or files
# * 💚 `💚` when fixing the CI build
# * ✅ `✅` when adding tests
# * 🔒 `🔒` when dealing with security
# * ⬆️ `⬆️` when upgrading dependencies
# * ⬇️ `⬇️` when downgrading dependencies
# * 👕 `👕` when removing linter warnings
8df2aaf
@kuychaco
Add RootController tess for `isValidCommit` 
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Don't forget the following emoji:
#
# * 🎨 `🎨` when improving the format/structure of the code
# * 🐎 `🐎` when improving performance
# * 🚱 `🚱` when plugging memory leaks
# * 📝 `📝` when writing docs
# * 🐧 `🐧` when fixing something on Linux
# * 🍎 `🍎` when fixing something on Mac OS
# * 🏁 `🏁` when fixing something on Windows
# * 🐛 `🐛` when fixing a bug
# * 🔥 `🔥` when removing code or files
# * 💚 `💚` when fixing the CI build
# * ✅ `✅` when adding tests
# * 🔒 `🔒` when dealing with security
# * ⬆️ `⬆️` when upgrading dependencies
# * ⬇️ `⬇️` when downgrading dependencies
# * 👕 `👕` when removing linter warnings
b329b41
@kuychaco
Allow any valid Git ref to be entered in OpenCommitDialog 
Don't restrict entries to valid commit shas. It works with branches, tags, and `HEAD` as well

# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Don't forget the following emoji:
#
# * 🎨 `🎨` when improving the format/structure of the code
# * 🐎 `🐎` when improving performance
# * 🚱 `🚱` when plugging memory leaks
# * 📝 `📝` when writing docs
# * 🐧 `🐧` when fixing something on Linux
# * 🍎 `🍎` when fixing something on Mac OS
# * 🏁 `🏁` when fixing something on Windows
# * 🐛 `🐛` when fixing a bug
# * 🔥 `🔥` when removing code or files
# * 💚 `💚` when fixing the CI build
# * ✅ `✅` when adding tests
# * 🔒 `🔒` when dealing with security
# * ⬆️ `⬆️` when upgrading dependencies
# * ⬇️ `⬇️` when downgrading dependencies
# * 👕 `👕` when removing linter warnings
Loading status checks…
5c851d7
@kuychaco
Add clarifying comment 
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Don't forget the following emoji:
#
# * 🎨 `🎨` when improving the format/structure of the code
# * 🐎 `🐎` when improving performance
# * 🚱 `🚱` when plugging memory leaks
# * 📝 `📝` when writing docs
# * 🐧 `🐧` when fixing something on Linux
# * 🍎 `🍎` when fixing something on Mac OS
# * 🏁 `🏁` when fixing something on Windows
# * 🐛 `🐛` when fixing a bug
# * 🔥 `🔥` when removing code or files
# * 💚 `💚` when fixing the CI build
# * ✅ `✅` when adding tests
# * 🔒 `🔒` when dealing with security
# * ⬆️ `⬆️` when upgrading dependencies
# * ⬇️ `⬇️` when downgrading dependencies
# * 👕 `👕` when removing linter warnings
Loading status checks…
d387391
@kuychaco
Fix borken test 
# 50-character subject line
#
# 72-character wrapped longer description. This should answer:
#
# * Why was this change necessary?
# * How does it address the problem?
# * Are there any side effects?
#
# Include a link to the ticket, if any.
#
# Don't forget the following emoji:
#
# * 🎨 `🎨` when improving the format/structure of the code
# * 🐎 `🐎` when improving performance
# * 🚱 `🚱` when plugging memory leaks
# * 📝 `📝` when writing docs
# * 🐧 `🐧` when fixing something on Linux
# * 🍎 `🍎` when fixing something on Mac OS
# * 🏁 `🏁` when fixing something on Windows
# * 🐛 `🐛` when fixing a bug
# * 🔥 `🔥` when removing code or files
# * 💚 `💚` when fixing the CI build
# * ✅ `✅` when adding tests
# * 🔒 `🔒` when dealing with security
# * ⬆️ `⬆️` when upgrading dependencies
# * ⬇️ `⬇️` when downgrading dependencies
# * 👕 `👕` when removing linter warnings
Loading status checks…
d66a68f
@smashwilson
Drop the long commit message and newline thresholds
Verified
This commit was signed with a verified signature.
@smashwilson smashwilson Ash Wilson
GPG key ID: 81B1DDB704F69D2A Learn about signing commits
Loading status checks…
ac3e0b4
@smashwilson
Bring abbreviatedBody()'s message up to date
Verified
This commit was signed with a verified signature.
@smashwilson smashwilson Ash Wilson
GPG key ID: 81B1DDB704F69D2A Learn about signing commits
Loading status checks…
9f6ad40
@smashwilson

smashwilson approved these changes Dec 11, 2018
View changes

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from In Progress 🔧 to QA Review 🔬 Dec 11, 2018

@kuychaco kuychaco merged commit 3c25540 into master Dec 11, 2018
2 checks passed

2 checks passed

codecov/patch Coverage not affected when comparing 4f3422f...9f6ad40
Details
codecov/project 90.72% (+0.99%) compared to 4f3422f
Details

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from QA Review 🔬 to Merged ☑️ Dec 11, 2018

This was referenced Jan 2, 2019

Closed
Closed
Closed

@smashwilson smashwilson deleted the ku-commit-item branch Jan 8, 2019

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