Merge button for PR screen #60
Conversation
| import { TouchableOpacity, StyleSheet, ImageBackground } from 'react-native'; | ||
|
|
||
| type Props = { | ||
| children: Object, |
components/Button/Button.js
Outdated
| @@ -8,16 +8,17 @@ import { UIText } from 'components'; | |||
| type Props = { | |||
| children: Object, | |||
| onPress: () => any, | |||
| style?: ComponentStyles | |||
| style?: ComponentStyles, | |||
| textStyle: Object | |||
components/Button/Button.js
Outdated
|
|
||
| return ( | ||
| <TouchableOpacity onPress={onPress} style={[styles.button, style]}> | ||
| <UIText style={styles.text}> | ||
| <UIText style={textStyle !== {} ? textStyle : styles.text}> |
| export default class ImageButton extends PureComponent<Props> { | ||
| render() { | ||
| const { onPress } = this.props; | ||
| const image = require('../../assets/threeDots.png'); |
There was a problem hiding this comment.
You can use import, but will be better to use vector icons, we already use https://github.com/oblador/react-native-vector-icons
|
|
||
| const styles = StyleSheet.create({ | ||
| imageBackground: { | ||
| width:50, height: 20, marginTop: 10, |
| styleText={styles.buttonText}> | ||
| Close PR | ||
| </ImageButton> | ||
| <Modal |
There was a problem hiding this comment.
Please use openModal instead of manually do modals
| ".js" | ||
| ] | ||
| } | ||
| "name": "ghubber", |
There was a problem hiding this comment.
Please revert ident for this file
Thanks!
There was a problem hiding this comment.
I'm not sure what to revert here?
There was a problem hiding this comment.
I mean, not needed to change this file (code indentation - spaces)
.eslintrc
Outdated
| @@ -37,6 +37,7 @@ | |||
| "Dispatch": true, | |||
| "Promise": true, | |||
| "ThunkAction": true, | |||
| "ActionType": true | |||
| "ActionType": true, | |||
| "require": true | |||
| @@ -88,6 +98,10 @@ class RepositoryPullRequestScreen extends PureComponent<Props> { | |||
| ]; | |||
| } | |||
|
|
|||
| setModalVisible(visible) { | |||
| @@ -40,7 +46,11 @@ function getStateColor(state: string): string { | |||
| } | |||
| } | |||
|
|
|||
| class RepositoryPullRequestScreen extends PureComponent<Props> { | |||
| class RepositoryPullRequestScreen extends PureComponent<Props, ModalState> { | |||
| state: ModalState = { | |||
| export default connect( | ||
| null, | ||
| { addModal } | ||
| )(ImageButton); |
There was a problem hiding this comment.
connecting a component to the redux store w/o props passing is the wrong way.
Bind dispatch to addModal where you're rendering this component and pass it as a prop here. But seems like you don't use it here, so remove connecting at all.
| @@ -0,0 +1,2 @@ | |||
|
|
|||
There was a problem hiding this comment.
Please, remove this unnecessary empty line
| @@ -101,6 +115,36 @@ class RepositoryPullRequestScreen extends PureComponent<Props> { | |||
| </View> | |||
| </View> | |||
| <UIText style={styles.body}>{pullRequest.body}</UIText> | |||
| <ImageButton | |||
| style={styles.button} | |||
| onPress={() => this.props.addModal( | |||
There was a problem hiding this comment.
Such kind of function passing is the anti-pattern.
Related reading
| data={[{ type: 'merge' }]} | ||
| renderOption={ | ||
| ({ item }) => { | ||
| return ( |
There was a problem hiding this comment.
You can omit it.
renderOption={({ item }) => (
<TouchableOpacity
key={item.type}
style={styles.modalContent}
onPress={() => { // <- remove this arrow function passing
closeModal();
}}
>
<Text style={styles.modalTitle}>Merge this pull request?</Text>
<Button
style={styles.deletePr}
textStyle={styles.deletePrText}
onPress={() => this.props.closeModal()} // <- remove this arrow function passing
>
Merge
</Button>
</TouchableOpacity>
)}But, I do prefer extract this rendering to a separate function.
I've made UI for now, and later I can configure/add redux and api actions for merging.