Skip to content

Conversation

@rolandjlevy
Copy link

No description provided.

.gitignore Outdated
/node_modules
/dist
.DS_Store
/images
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect images should be included in repo otherwise someone looking at your repo will not have access to them

receiveInput (text) {
this.setState({
searchQuery: text,
searchDisplay: text.length > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than storing searchDisplay in state, it might be better to compute it in the render method as it's a derived property which depends on the searchQuery value.


// receive focus event from Search component
receiveFocus () {
this.setState({ searchDisplay: this.state.searchQuery.length > 0})
Copy link
Contributor

Choose a reason for hiding this comment

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

If searchDisplay is computed in render method, we can set a focused property in state here

@@ -0,0 +1,82 @@
// http://www.omdbapi.com/?s=batman&apikey=2cda7206
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 file is not in use and can go into .gitignore


render () {
return (
<div dangerouslySetInnerHTML={{__html: this.props.children}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unnecessary. It looks like this is only used to wrap the h3 in a div. Why not do it directly in MovieDisplay component?

@dmitrigrabov
Copy link
Contributor

Really nice work

rolandjlevy and others added 30 commits December 2, 2018 13:33
Updated API URL - removed http: due to mixed content error
Updated webpack with CleanWebpackPlugin
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