Refactoring builders [do not merge] #1919
Conversation
…urn a placeholder filepatch.
Co-Authored-By: Vanessa Yuen <vanessayuenn@users.noreply.github.com>
...not sure how we would get into this situation, but it seems like an important case to cover.
unnecessary console output during tests is a bummer.
| @@ -19,6 +19,7 @@ export default class PullRequestReviewsController extends React.Component { | |||
| ), | |||
| }), | |||
| getBufferRowForDiffPosition: PropTypes.func.isRequired, | |||
| isPatchTooLargeOrCollapsed: PropTypes.func.isRequired, | |||
There was a problem hiding this comment.
this is a really long name -- can you come up with something more concise?
There was a problem hiding this comment.
how about just isPatchCollapsed?
| } | ||
| } | ||
|
|
||
| isPatchTooLargeOrCollapsed = filePatchPath => { |
There was a problem hiding this comment.
same as above - this name is too long and the line length linters are gonna be unhappy.
| }; | ||
| return shallow( | ||
| <PullRequestCommentsView | ||
| isPatchTooLargeOrCollapsed={sinon.stub().returns(false)} |
There was a problem hiding this comment.
again, this variable name is too long
| @@ -16,7 +16,7 @@ export default class CommitDetailController extends React.Component { | |||
| this.state = { | |||
| messageCollapsible: this.props.commit.isBodyLong(), | |||
| messageOpen: !this.props.commit.isBodyLong(), | |||
| }; | |||
| } | |||
There was a problem hiding this comment.
mind adding the missing semicolon here?
| @@ -42,12 +49,32 @@ export default class FilePatchHeaderView extends React.Component { | |||
| <header className="github-FilePatchView-header"> | |||
| <span className="github-FilePatchView-title"> | |||
| {this.renderTitle()} | |||
| {this.renderCollapseButton()} | |||
There was a problem hiding this comment.
Should we move the chevron icon to the left? So that the position doesn't jump when there are more buttons. Alternative would be to move it all the way to the right.
| export const DEFAULT_OPTIONS = { | ||
| // Number of lines after which we consider the diff "large" | ||
| // TODO: Set this based on performance measurements | ||
| largeDiffThreshold: 100, |
There was a problem hiding this comment.
@simurai: how many lines do you think constitutes a large diff? Not just from a performance perspective, but from a user experience perspective. Like how many lines is disruptive to a user when they're trying to read, because often large diffs are the result of auto generated code.
There was a problem hiding this comment.
Hmmm.. will large diffs be collapsed by default or there is a "load" button? Maybe if the diff is so large that it fills the whole scroll height. Then I can uncollapse only if I'm really interested in that file. 100 seems fine. 👍
There was a problem hiding this comment.
@vanessayuenn @kuychaco @smashwilson care to weigh in?
There was a problem hiding this comment.
mm we were using 1000 lines as the threshold before, but that's for performance reasons. 100 does seem a bit small though considering it's counting both deleted and added lines. 🤔
300 seems a bit more reasonable, but still an arbitrary number.
There was a problem hiding this comment.
but still an arbitrary number.
Would this be good as a package option then? Pick a value that should work for most users, but let people who need to configure it. Also, would that be something that can be refined by metrics?
Codecov Report
@@ Coverage Diff @@
## master #1919 +/- ##
=========================================
- Coverage 92.1% 90.81% -1.3%
=========================================
Files 189 190 +1
Lines 10783 11184 +401
Branches 1580 1624 +44
=========================================
+ Hits 9932 10157 +225
- Misses 851 1027 +176
Continue to review full report at Codecov.
|
| @@ -19,6 +19,7 @@ export default class PullRequestReviewsController extends React.Component { | |||
| ), | |||
| }), | |||
| getBufferRowForDiffPosition: PropTypes.func.isRequired, | |||
| isPatchTooLargeOrCollapsed: PropTypes.func.isRequired, | |||
There was a problem hiding this comment.
how about just isPatchCollapsed?
| @@ -27,13 +29,26 @@ export default class ChangedFileContainer extends React.Component { | |||
| constructor(props) { | |||
| super(props); | |||
| autobind(this, 'fetchData', 'renderWithData'); | |||
|
|
|||
| this.lastMultiFilePatch = null; | |||
| this.sub = new CompositeDisposable(); | |||
There was a problem hiding this comment.
one line comment -- not part of a review.
| @@ -1,6 +1,7 @@ | |||
| import React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import yubikiri from 'yubikiri'; | |||
| import {CompositeDisposable} from 'event-kit'; | |||
This pull request extracts a PatchBuffer out of the common code in (three) places we build up TextBuffers with Markers organized into MarkerLayers (hunks, patches, additions, deletions, unchanged lines...). It also adds operations to the PatchBuffer model that will let us build them and manipulate them as you expand and collapse individual patches, preserving marker locations.
Also, we changed
test/builders/patch.jsto build "raw" what-the-diff output and use the "real" builders to build model objects and getting our tests green where those are used.