Skip to content
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

Open
wants to merge 61 commits into
base: master
Choose a base branch
from

Conversation

rolandjlevy
Copy link

No description provided.

.gitignore Outdated
@@ -1,3 +1,4 @@
/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:https://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.

None yet

2 participants