-
Notifications
You must be signed in to change notification settings - Fork 36
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
Friday version of React Cinema #9
base: master
Are you sure you want to change the base?
Conversation
.gitignore
Outdated
@@ -1,3 +1,4 @@ | |||
/node_modules | |||
/dist | |||
.DS_Store | |||
/images |
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 expect images should be included in repo otherwise someone looking at your repo will not have access to them
src/components/App.js
Outdated
receiveInput (text) { | ||
this.setState({ | ||
searchQuery: text, | ||
searchDisplay: text.length > 0 |
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.
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.
src/components/App.js
Outdated
|
||
// receive focus event from Search component | ||
receiveFocus () { | ||
this.setState({ searchDisplay: this.state.searchQuery.length > 0}) |
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.
If searchDisplay
is computed in render method, we can set a focused
property in state here
src/components/LocalStorage-v2.js
Outdated
@@ -0,0 +1,82 @@ | |||
// https://www.omdbapi.com/?s=batman&apikey=2cda7206 |
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.
It looks like this file is not in use and can go into .gitignore
|
||
render () { | ||
return ( | ||
<div dangerouslySetInnerHTML={{__html: this.props.children}}> |
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.
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?
Really nice work |
Changes for deployment
Webpack bugs now fixed
Updated API URL - removed http: due to mixed content error
Heruko deployment
Updated webpack with CleanWebpackPlugin
No description provided.