Skip to content

Conversation

@zuberman
Copy link

@zuberman zuberman commented Oct 3, 2018

No description provided.

this.receiveVideoBlock = this.receiveVideoBlock.bind(this);
}

componentDidMount(){
Copy link
Contributor

Choose a reason for hiding this comment

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

method is not in use and can be removed


receivePaginationQuery(){
this.setState({
pageNumber: ++this.state.pageNumber
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling ++ mutates state. It would be better to use this.state.pageNumber + 1 to increment value

constructor(){
super();

this.state = {
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this is not in use and can be removed

})
let searchEntry = event.target.value
if (searchEntry.length > 2){
handleSubmit(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

you are passing a change event into a submit event handler which might make things go funny. It would be better to split out functionality you want to share between two event handlers into own function and call that from both event handlers

console.log(body)
this.setState({
modalDetails: body,
modalClass: "modal2"
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be better to make this property a boolean and set the actual class name in render method. the reason for that is that if you wanted to change the class name in future, you would need to do it in mutiple places. if you set the class name in render depending on the state of the boolean, you would only need to change it in one place

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants