-
Notifications
You must be signed in to change notification settings - Fork 787
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
feat(commit): view commits #258
Changes from 10 commits
9641e6f
a5daf02
d726352
fafb8c7
867e45a
3c48cad
7173eb6
9c3e9c7
e4a0533
e8e079b
1a7b810
0ec5a59
e5a57e5
1f2fb2c
d0c8c4e
94cbfd4
7ae9e85
485cf83
365cae5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import React from 'react'; | ||
import { StyleSheet, TouchableHighlight, View } from 'react-native'; | ||
import { ListItem } from 'react-native-elements'; | ||
import { colors, fonts } from 'config'; | ||
|
||
type Props = { | ||
commit: Object, | ||
navigation: Object, | ||
}; | ||
|
||
const styles = StyleSheet.create({ | ||
container: { | ||
flexDirection: 'row', | ||
alignItems: 'center', | ||
justifyContent: 'center', | ||
paddingRight: 10, | ||
paddingVertical: 5, | ||
borderBottomWidth: 1, | ||
borderBottomColor: colors.greyLight, | ||
}, | ||
listItemContainer: { | ||
flex: 1, | ||
borderBottomWidth: 0, | ||
}, | ||
title: { | ||
color: colors.primaryDark, | ||
...fonts.fontPrimary, | ||
}, | ||
}); | ||
|
||
export const CommitListItem = ({ commit, navigation }: Props) => | ||
<TouchableHighlight | ||
onPress={() => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please move this arrow function out of this prop? Maybe you can create a function outside of the component and call it here instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice I'm fine either way honestly, but I do see the benefit of having methods separate 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having an arrow function inside a prop may become a perf issue. |
||
navigation.navigate('Commit', { | ||
commit, | ||
})} | ||
underlayColor={colors.greyLight} | ||
> | ||
<View style={styles.container}> | ||
<ListItem | ||
containerStyle={styles.listItemContainer} | ||
title={commit.commit.message.split('\n')[0]} | ||
titleNumberOfLines={1} | ||
subtitle={ | ||
`${commit.sha.substring(0, 7)} - ${commit.commit.author.name}` || '' | ||
} | ||
leftIcon={{ | ||
name: 'git-commit', | ||
size: 36, | ||
color: colors.grey, | ||
type: 'octicon', | ||
}} | ||
hideChevron | ||
titleStyle={styles.title} | ||
/> | ||
</View> | ||
</TouchableHighlight>; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,11 @@ | ||
import React, { Component } from 'react'; | ||
import { StyleSheet, View, ActivityIndicator } from 'react-native'; | ||
import { | ||
Text, | ||
StyleSheet, | ||
View, | ||
ActivityIndicator, | ||
TouchableHighlight, | ||
} from 'react-native'; | ||
import { ListItem, Button } from 'react-native-elements'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry, I was fixing merge conflicts here and included this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Np mate, thanks for the conflict fix 😄 |
||
import Parse from 'parse-diff'; | ||
import moment from 'moment/min/moment-with-locales.min'; | ||
|
@@ -35,9 +41,10 @@ const styles = StyleSheet.create({ | |
}, | ||
diffBlocksContainer: { | ||
flexDirection: 'row', | ||
alignItems: 'center', | ||
justifyContent: 'flex-end', | ||
alignItems: 'stretch', | ||
justifyContent: 'space-between', | ||
paddingRight: 10, | ||
paddingLeft: 10, | ||
paddingBottom: 10, | ||
}, | ||
badge: { | ||
|
@@ -67,15 +74,33 @@ export class IssueDescription extends Component { | |
props: { | ||
issue: Object, | ||
diff: string, | ||
commits: Array, | ||
isMerged: boolean, | ||
isPendingDiff: boolean, | ||
isPendingCommit: boolean, | ||
isPendingCheckMerge: boolean, | ||
onRepositoryPress: Function, | ||
userHasPushPermission: boolean, | ||
language: string, | ||
navigation: Object, | ||
}; | ||
|
||
navigateToCommitList = () => { | ||
const { commits } = this.props; | ||
|
||
if (commits.length > 1) { | ||
this.props.navigation.navigate('CommitList', { | ||
title: translate('repository.commitList.title', this.props.language), | ||
commits, | ||
}); | ||
} else { | ||
this.props.navigation.navigate('Commit', { | ||
commit: commits[0], | ||
title: commits[0].sha.substring(0, 7), | ||
}); | ||
} | ||
}; | ||
|
||
renderLabelButtons = labels => { | ||
return labels | ||
.slice(0, 3) | ||
|
@@ -86,8 +111,10 @@ export class IssueDescription extends Component { | |
const { | ||
diff, | ||
issue, | ||
commits, | ||
isMerged, | ||
isPendingDiff, | ||
isPendingCommit, | ||
isPendingCheckMerge, | ||
onRepositoryPress, | ||
userHasPushPermission, | ||
|
@@ -149,9 +176,20 @@ export class IssueDescription extends Component { | |
|
||
{issue.pull_request && | ||
<View style={styles.diffBlocksContainer}> | ||
{isPendingCommit && | ||
<ActivityIndicator animating={isPendingCommit} size="small" />} | ||
|
||
{isPendingDiff && | ||
<ActivityIndicator animating={isPendingDiff} size="small" />} | ||
|
||
{!isPendingCommit && | ||
<TouchableHighlight | ||
onPress={() => this.navigateToCommitList()} | ||
underlayColor={colors.greyLight} | ||
> | ||
<Text>{`${commits.length} commits`}</Text> | ||
</TouchableHighlight>} | ||
|
||
{!isPendingDiff && | ||
(lineAdditions !== 0 || lineDeletions !== 0) && | ||
<DiffBlocks | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should not go here since we're not using
@flow
. Thoughts @Antoine38660 @housseindjirdeh ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope quite a few components are using flow types :) I think it's definitely nice to have and hopefully we can increase type coverage throughout the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, we don't have
@flow
on top...so flow should not be checking anything. Or am I just missing something here?