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

Add tab for all changed files in a pull request #1829

Merged
merged 104 commits into from Dec 18, 2018

Conversation

@annthurium
Copy link
Contributor

annthurium commented Dec 7, 2018

Description of the Change

Another step in our quest to march ever onwards towards code review in the editor.

This pull request adds a tab to the pull request pane where we show all changed files associated with a pull request.

Clicking on the "Files" tab displays the full, multi-file diff associated with the pull request> . This is similar to the "Files Changed" tab on dotcom. See design here: https://github.com/atom/github/blob/master/docs/feature-requests/003-pull-request-review.md

When we have support for comments, we will display comments inline in the files view. Comments are out of scope for this pull request, but they're one of the next things we're gonna tackle.

Alternate Designs

  • See https://github.com/atom/github/blob/master/docs/feature-requests/003-pull-request-review.md for alternative visual designs

  • For fetching the data, it wasn't available from the graphQL api unless we want to fetch the diff for each individual commit, and then calculate the combined diff from those, which would be potentially slow af for pull requests with many commits. There is a .diff URL, but you can't use that if the repo is private. Ultimately we went with the REST api from github dotcom for fetching this data.

Benefits

  • Gets us closer to code review in the editor.
  • Less context switching for users
  • We have added some Endpoint abstractions for fetching data from dotcom. that get us closer to Enterprise support.

Possible Drawbacks

In general, code review in the editor adds a non trivial amount complexity to the UI, which is only justified for users that use GitHub pull request reviews.

Applicable Issues

N/A

TODO

Implementation tracking project

QA Checklist

  • Pull request detail view:
    • contains the new "Files" tab.
    • No staging / unstaging / jump to file buttons present on Files tab (including for symlink and file mode changes)
    • network connectivity errors don't crash the whole pane, instead showing an error message contained within the tab.
    • Viewing a pull request that doesn't have a HEAD repository still works.
    • Pull request is able to be checked out.
    • Styling of commits in commit tab is consistent with what's on master.
  • Issues can be viewed
  • issue and pull requests render reasonably at wide widths / large monitors
  • issues and pull requests now have a fixed header that stays at the top when a user scrolls
    - [ ] issue and pull requests collapse reasonably at narrow widths without looking squished or weird ehh, not a ship blocker.
  • issues and pull requests don't look toooooooo 馃挬 when docked.

Metrics

  • Number of times the changed files tab was viewed.
  • Number of times users jumped from a file in the changed files tab to the full version of the file.

Release Notes

This pull request adds a tab to the pull request pane in the GitHub package, that shows all changed files in a pull request.

User Experience Research (Optional)

  • Are users able to open the pull request view?
  • Are users able to find the new tab?

View rendered docs/react-component-atlas.md

@annthurium annthurium changed the title Add tab for all changed files in a pull request [wip] Add tab for all changed files in a pull request Dec 7, 2018

@kuychaco kuychaco changed the base branch from master to ku-commit-item Dec 7, 2018

annthurium and others added some commits Dec 8, 2018

Making fetch happen
It finally happened!

Also made the React component hierarchy and shit.
make patch URL actually valid
I was testing error cases and forgot to fix it, oops.
pass `itemType` to all the components that need it
Co-Authored-By: Katrina Uychaco <katrina@github.com>
jeez, you want more props? so needy.
Co-Authored-By: Katrina Uychaco <katrina@github.com>
smell ya later unnecessary div
Co-Authored-By: Katrina Uychaco <katrina@github.com>
@todo

This comment was marked as outdated.

Copy link

todo bot commented Dec 11, 2018

it would probably be better to use absolute positioning

// TODO: it would probably be better to use absolute positioning
// and then just set top / left / bottom right, and also to make
// the whole tab container responsive for bigger screens.
&-filesChanged {
height: 100vh;
}


This comment was generated by todo based on a TODO comment in acd9626 in #1829. cc @atom.

@smashwilson smashwilson changed the base branch from ku-commit-item to master Dec 11, 2018

smashwilson added some commits Dec 12, 2018

@todo

This comment has been minimized.

Copy link

todo bot commented Dec 12, 2018

centralize endpoint translation logic between here and lib/relay-network-layer-manager.js.

// TODO centralize endpoint translation logic between here and lib/relay-network-layer-manager.js.
// Maybe move it to the Remote model instead?
const endpoint = this.props.host === 'github.com' ? 'https://api.github.com' : this.props.host;
return `${endpoint}/repos/${this.props.owner}/${this.props.repo}/pulls/${this.props.number}`;
}


This comment was generated by todo based on a TODO comment in 39bb8b9 in #1829. cc @atom.
@smashwilson
Copy link
Member

smashwilson left a comment

Looking good!

I found a few non-essential tidiness things that we can fix or not fix, but that won't actually break anything. Other than that I think we're good.


componentDidUpdate(prevProps) {
if (this.props.shouldRefetch && !prevProps.shouldRefetch) {
this.setState({isLoading: true, error: null});

This comment has been minimized.

@smashwilson

smashwilson Dec 17, 2018

Member

Always makes me nervous when I see setState used synchronously. I think it's okay here because we don't actually need to wait for the re-render with {isLoading: true} to complete before we start fetching the updated diff.

},
// eslint-disable-next-line handle-callback-err
}).catch(err => {
diffError(`Network error encountered at fetching ${url}`);

This comment has been minimized.

@smashwilson

smashwilson Dec 17, 2018

Member

It might be nice to report err to console.error() or something rather than dropping it on the floor, to help us diagnose if a user hits this catch block unexpectedly. (Most of the time I'd expect it to be something like "you're not actually on wifi" but just in case...)

diffError(`Unable to fetch diff for this pull request${response ? ': ' + response.statusText : ''}.`);
}
} catch (e) {
diffError('Unable to parse diff for this pull request.');

This comment has been minimized.

@smashwilson

smashwilson Dec 17, 2018

Member

Same here 鈽濓笍

Show resolved Hide resolved lib/containers/remote-container.js Outdated
.github-FilePatchView {
font-size: 1.1em;
}

This comment has been minimized.

@smashwilson

smashwilson Dec 17, 2018

Member

Interesting. I never even noticed this was here 馃槅

@@ -47,10 +47,8 @@ export default class GitHubTabView extends React.Component {

if (this.props.currentRemote.isPresent()) {
// Single, chosen or unambiguous remote
// only supporting GH.com for now, hardcoded values

This comment has been minimized.

@smashwilson

smashwilson Dec 17, 2018

Member

We are still supporting dotcom... but the Endpoint stuff does help lay the groundwork for actual Enterprise-readiness.

@@ -169,7 +169,7 @@ export default class MultiFilePatchView extends React.Component {
render() {
const rootClass = cx(
'github-FilePatchView',
`github-FilePatchView--${this.props.stagingStatus}`,
{[`github-FilePatchView--${this.props.stagingStatus}`]: this.props.stagingStatus},

This comment has been minimized.

@smashwilson

smashwilson Dec 17, 2018

Member

馃憤 Ah, good, no more github-FilePatchView--undefined.

</Tab>
</TabList>
{/* 'Files Changed' and 'Reviews' tabs to be added in the future. */}
{/* 'Reviews' tab to be added in the future. */}

This comment has been minimized.

@smashwilson

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from In Progress 馃敡 to QA Review 馃敩 Dec 17, 2018

smashwilson and others added some commits Dec 17, 2018

@annthurium annthurium merged commit 475b33d into master Dec 18, 2018

2 checks passed

codecov/patch 99.12% of diff hit (target 90.85%)
Details
codecov/project 91.13% (+0.27%) compared to a10a8b3
Details

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from QA Review 馃敩 to Merged 鈽戯笍 Dec 18, 2018

@annthurium annthurium deleted the 18-dec-files-changed-tab branch Dec 18, 2018

simurai added a commit that referenced this pull request Dec 19, 2018

Update README screenshot
With the changes from #1829
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment