split IssueishDetailView into IssueDetailView and PullRequestDetailView #1804
Conversation
you can't check out issues, dawg.
I don't think we need this? But I could be wrong. graphql still confuses me.
...I think these props are actually not required in this component. Though, they are required in the child component, and fetched by relay.
vanessayuenn
left a comment
vanessayuenn
left a comment
There was a problem hiding this comment.
thanks for tackling this! 
It feels a bit dirty to be checking in multiple places, and also that both the issue and the pullRequest have a typename. I couldn't figure out how to pull the `__typename` up a level in graphql, but at least having a single source of truth in state for the type of the type of item we're dealing with seems marginally cleaner.
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
Okay, now I am being that jerk to come in and request changes with the third review 🙈
The only showstopper I'm pointing out here is the state derivation being done with a deprecated React lifecycle method (marked with 🚨). Everything else is just my usual verbose self 😄
| headRemote.getOwner() === repository.owner.login && | ||
| headRemote.getRepo() === repository.name && | ||
| headPush.getShortRemoteRef() === `pull/${issueish.number}/head`; | ||
| headPush.getShortRemoteRef() === `pull/${pullRequest.number}/head`; |
There was a problem hiding this comment.
Okay, yeah, and this is guaranteed to be defined because we gate on this.state.typename up above. 👍
| HEART: '❤️', | ||
| }; | ||
|
|
||
| export default class EmojiReactionsView extends React.Component { |
There was a problem hiding this comment.
Ahhh you extracted this from IssueishDetailView? Yesss 💯
| issue={issue} | ||
| switchToIssueish={this.props.switchToIssueish} | ||
| /> | ||
| </Fragment> |
| </div> | ||
| </div> | ||
| {isPr && this.renderPrMetadata(issueish, repo)} | ||
| {this.renderPrMetadata(pullRequest, repo)} |
There was a problem hiding this comment.
Getting rid of all of this conditional rendering is great.
|
|
||
| recordOpenInBrowserEvent() { | ||
| addEvent('open-issueish-in-browser', {package: 'github', from: 'issueish-header'}); | ||
| addEvent('open-pull-request-in-browser', {package: 'github', component: this.constructor.name}); |
There was a problem hiding this comment.
Oh, cool, we get better granularity on these telemetry events, too.
Do we need to update anything or let @telliott27 know that this event name is changing?
There was a problem hiding this comment.
gooood question. I was just thinking about that during our meeting this morning.
@telliott27 : will this break the existing dashboard queries?
|
|
||
| omitIssueish: false, | ||
| omitPullRequestData: false, | ||
| omitIssueData: false, |
There was a problem hiding this comment.
Speaking as the one who put them there, I don't love these factories, btw 💔. I'm not sure what a decent alternative to constructing these complicated prop structures that Relay gives us is, though. More builders instead... ?
There was a problem hiding this comment.
yeah, this one is especially bad because to render an issue to test, you still need pull request data with a typename of Issue. I also couldn't think of a better way to handle it, though.
`componentDidMount` is deprecated, oops. Thought about using memoize-one, but we're memoizing to avoid a lengthly chained lookup in code, rather than because it's an expensive computation, so having a memoization helper lib isn't actually all that useful here.
addresses #1715.
These components have enough separate functionality that we should move them into their own files. There should be no user facing changes as a result of this refactor - it just makes it easier for us to maintain things in the future.
Todo
IssueishDetailViewintoPullRequestDetailViewIssueishDetailViewintoIssueDetailViewIssueishDetailContainerand any other dead codeManual test plan: