Skip to content
This repository was archived by the owner on Aug 11, 2020. It is now read-only.

Conversation

@djuric42
Copy link

@djuric42 djuric42 commented Feb 1, 2018

Pull request needs refactoring and improvements suggestions
Follow:

  • Be familiar with team commits
  • Be familiar with git-flow principles

Goal of pull request is to technically improve new code and to perform team rules while contributing

constructor () {
super()
constructor() {
super();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not use the semicolon(;) at the end of a line

// Event Handlers
onCheckBoxPress () {
if (this.state.checked) {
pressCheckbox(){
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow function name convention
functionName () is correct
functionName() is not correct

backgroundColorLeft: {backgroundColor: 'rgba(255,255, 255, 0.2)', borderColor: 'rgba(255,255, 255, 0)'},
backgroundColorRight: {backgroundColor: 'rgba(255,255, 255, 0)', borderColor: 'rgba(255,255, 255, 0.2)'}
}
// this.state = {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete commented code if it's not TODO or similar

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, every component should have the constructor, usually state should be managed from there

this.setState({btnTxt: 'Take a photo'})
const currentStep = this.state.step
if (currentStep === 1) {
this.setState({btnTxt: 'Take a photo'})
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setState is asynchronous
It’s uncommon to call setState multiple times from the same block of code (e.g. an event callback)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var camera = this.props.showDocCamera ?
<Camera style={{ flex: 1 }} type={this.state.type} ref={ref => {this.camera = ref}}>
<View style={{ flex: 1, backgroundColor: 'transparent', flexDirection: 'row',}}>
<TouchableOpacity style={{ flex: 0.1, alignSelf: 'flex-end', alignItems: 'center',}}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not apply styles in DOM directly, because of rendering parser

We keep styles nice if:

  • By moving styles away from the render function, you're making the code easier to understand.
  • Naming the styles is a good way to add meaning to the low level components in the render function.

<Text style={styles.text}>Please center your passport in the area above. Ensure that there’s enough light in the room for better picture quality.</Text>
<View>
{/* <TouchableOpacity style={styles.button}
{/*<TouchableOpacity style={styles.button}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove not useful comments

@djuric42 djuric42 assigned creativenet and unassigned creativenet Feb 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants