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.

Sign up for GitHub

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

React Cinema - Ralph #5

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

React Cinema - Ralph #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2792f13
Added search
Jun 22, 2018
6a6db11
Added infinite scroll
Jun 23, 2018
4be3665
Update style movies list to two columns
Jun 23, 2018
01b1b79
Added Back To Top button
Jun 23, 2018
05becbb
Refactoring Loading and BackToTop into components
Jun 23, 2018
d7f4544
Added Info button
Jun 23, 2018
f2f47ad
Bug fix placeholder image
Jun 23, 2018
5a4b372
Added watch list
Jun 23, 2018
fe92891
Added watchList to localStorage
Jun 24, 2018
a151f78
Working on localStorage and watchList
Jun 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8" />
<link rel="stylesheet" src="./style.css">
<title>Hello World</title>
</head>
<body>
<div id="root"></div>
<script src="dist/bundle.js"></script>
</body>
</html>

<head>
<meta charset="UTF-8" />
<link rel="stylesheet" href="./src/css/style.css">
<title>OMDb Movie search</title>
</head>

<body>
<div id="root"></div>
<script src="dist/bundle.js"></script>
</body>

</html>
136 changes: 132 additions & 4 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,142 @@
import React from 'react';

import Header from './Header';
import Search from './Search';
import Movies from './Movies';
import Loading from './elements/Loading';
import BackToTop from './elements/BackToTop';


class App extends React.Component {
constructor(){
constructor() {
super();
this.state = {
search: [],
searchTerm: "",
page: 2,
watchList: localStorage.getItem("watchList") ? localStorage.getItem("watchList") : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

You may need to perform JSON.parse on item retrieved from localStorage otherwise I expect it will just be a string

watchListSection: false
}
this.receiverSearch = this.receiverSearch.bind(this);
this.receiverWatchList = this.receiverWatchList.bind(this);
this.receiverDisplayWatchlist = this.receiverDisplayWatchlist.bind(this);
this.handleScroll = this.handleScroll.bind(this);
}

handleScroll() {
const windowHeight = "innerHeight" in window ? window.innerHeight : document.documentElement.offsetHeight;
const body = document.body;
const html = document.documentElement;
const docHeight = Math.max(body.scrollHeight, body.offsetHeight, html.clientHeight, html.scrollHeight, html.offsetHeight);
const windowBottom = windowHeight + window.pageYOffset;
const backToTop = body.querySelector("#back-to-top");

const loading = document.querySelector('#search-results__loading');
const OMBbAPIKey = "aba7af8e";
const query = 'http://www.omdbapi.com/?s=' + this.state.searchTerm + '&apikey=' + OMBbAPIKey + '&page=' + this.state.page;

// Back to top button
if (body.scrollTop > 300 || html.scrollTop > 300) {
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 manipulating the DOM by adding classes, it would be better to set property in state based on scrollTop and set the class using JSX

backToTop.classList.add('visible');
} else {
backToTop.classList.remove('visible');
}

// Infinite scroll
if (windowBottom >= docHeight) {
fetch(query)
.then(response => {
loading.textContent = "Loading...";
Copy link
Contributor

Choose a reason for hiding this comment

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

As with previous comment, it's good practice to avoid manipulating DOM directly and let JSX handle it. Let me know if you want me to help with this

loading.classList.add('show');
return response.json();
}
)
.then(movies => {
if (movies.Search) {
const self = this;
setTimeout(function () {
self.setState({
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid using self by using an arrow function. It will set the value of this to what it is in parent scope.

search: [...self.state.search, ...movies.Search],
page: self.state.page += 1
});
loading.classList.remove('show');
}, 500)
} else {
loading.textContent = "No more results";
loading.classList.add('show');
}
})
.catch(error => {
console.log(error);
loading.textContent = "There's been a problem fetching the results. Try again later..";
loading.classList.add('show');
});

}
}

componentDidMount() {
window.addEventListener("scroll", this.handleScroll);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

}

receiverSearch(movies, searchTerm) {
this.setState({
search: movies,
searchTerm: searchTerm,
watchListSection: false
})
}

receiverDisplayWatchlist() {
this.setState({
watchListSection: true
})
}

receiverWatchList(articleTitle, action) {
if (action === "addToWatchList") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest implementing this as two functions. One will do addToWatchList and another deleteFromWatchList

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping function to a single purpose will keep manageable and prevent them from becoming too large and doing too many things

this.state.search.forEach(articleObj => {
return Object.keys(articleObj).find(articleKey => {
if (articleObj[articleKey] === articleTitle) {
this.setState({
watchList: [...this.state.watchList, articleObj]
})

localStorage.getItem("watchList")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using localStorage multiple times here, I would suggest using variables to store value from localStorage, update it depending on value found and write updated value back. It will avoid excess reads and make it easier to follow. For example

const watchList = localStorage.getItem("watchList");

const updatedWatchList = watchList ? [articleObj, [...JSON.parse(watchList)] : [...this.state.watchList, articleObj];
 
localStorage.setItem("watchList", JSON.stringify(updatedWatchList));

? localStorage.setItem("watchList", JSON.stringify([articleObj, ...JSON.parse(localStorage.getItem("watchList"))]))
: localStorage.setItem("watchList", JSON.stringify([...this.state.watchList, articleObj]));
}
});
});
} else {
const index = JSON.parse(this.state.watchList).forEach((item, index) => {
if (item.Title === articleTitle) {
// console.log("Aquis", this.state.watchList);
this.setState({
watchList: JSON.parse(this.state.watchList).splice(index, 1)
})
localStorage.setItem("watchList", this.state.watchList);
// console.log("Aquis", localStorage.getItem("watchList"));
}
});
}
}

render(){
render() {
const watchList = this.state.watchListSection;
let MoviesList;
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid using capitals for things which are not classes or component names

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that in this case it will contain a component instance, but it will be an instance of the class rather than the class itself. For example

const movie = new Movie();

if (watchList) {
MoviesList = <Movies movies={localStorage.getItem("watchList") ? JSON.parse(localStorage.getItem("watchList")) : this.state.watchList} receiver={this.receiverWatchList} watchlistbutton={false} />;
} else {
MoviesList = <Movies movies={this.state.search} receiver={this.receiverWatchList} watchlistbutton={true} />;
}
return (
<div>
React cinema app
<div className="container">
<Header title="OMDb Movie search" receiver={this.receiverDisplayWatchlist} />
<Search receiver={this.receiverSearch} />
{MoviesList}
<Loading />
<BackToTop />
</div>
)
}
Expand Down
21 changes: 21 additions & 0 deletions src/Header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import React from 'react';
class Header extends React.Component {
constructor(props) {
super(props);
this.handleWatchList = this.handleWatchList.bind(this);
}
handleWatchList() {
document.querySelectorAll('.movie-info__moreinfo').forEach(item => item.classList.remove("disabled"));
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 handle this using JSX rather than DOM manipulation as a re-render could blow out your changes and create some strange bugs

this.props.receiver();
}
render() {
return (

<header className="main-header">
<h1 className="main-heading">{this.props.title}</h1>
<button onClick={this.handleWatchList} className="movie-info__moreinfo watch-list__button">Watch List</button>
</header>
)
}
}
export default Header;
91 changes: 91 additions & 0 deletions src/Movie.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import React from 'react';

class Movie extends React.Component {
constructor(props) {
super(props);
this.movieFetch = this.movieFetch.bind(this);
this.handleWatchList = this.handleWatchList.bind(this);
};
movieFetch(e) {
const evtTarget = e.target;
const movieParentNode = evtTarget.parentNode;
const OMBbAPIKey = "aba7af8e";
fetch('http://www.omdbapi.com/?t=' + this.props.title + '&apikey=' + OMBbAPIKey)
.then(response => {
return response.json();
})
.then(data => {
// Show/hide the list
if (movieParentNode.parentNode.querySelector(".movie-info__moreinfo-details")) {
// Remove list
movieParentNode.parentNode.querySelector(".movie-info__moreinfo-details").remove();
// Update button text
evtTarget.textContent = "Info";
} else {
// Create list with desired movie details
const infoToDisplay = [
"Genre",
"Plot",
"Runtime",
"Awards",
"Language"
];
const html = Object.keys(data)
.map(function (key) {
if (infoToDisplay.indexOf(key) !== -1) {
return `<li><strong>${key}</strong>: ${data[key]}</li>`;
}
})
.join("");

// Update button text
evtTarget.textContent = "Close";

// Append list
const moreInfoList = document.createElement("ul");
moreInfoList.setAttribute("class", "movie-info__moreinfo-details");
moreInfoList.innerHTML = html;
movieParentNode.parentNode.append(moreInfoList);
}
})
.catch(error => {
console.error(error);
});
}
handleWatchList(e, action) {
const evtTarget = e.target;
if (!evtTarget.classList.contains('disabled')) {
evtTarget.classList.add("disabled");
const articleTitle = evtTarget.getAttribute("data-title");
this.props.receiver(articleTitle, action);
}
}
render() {
const watchlistbutton = this.props.watchlistbutton;
return (
<div id="movie-details" className="movie-details">
<div className='movie-poster'>
<img src={this.props.poster === 'N/A' ? 'http://via.placeholder.com/150x220?text=Image' : this.props.poster} alt={this.props.title} className='movie-poster__image' />
</div>
<div className='movie-info'>
<h2 className='movie-info__title'>{this.props.title}</h2>
<div className='movie-info__year'><strong>Year:</strong> {
this.props.year
}</div>
<a className='movie-info__imdb-link' target='_blank' href={'https://www.imdb.com/title/' + this.props.imdbID} >IMDb &#8599;</a>
<a id=''
className='movie-info__imdb-link'
target="_blank" href={'https://www.youtube.com/results?search_query=' + this.props.title + " " + this.props.year + " trailer"}>
Watch trailer &#8599;
</a>
{watchlistbutton
? <button id='' onClick={(e) => this.handleWatchList(e, "addToWatchList")} className='movie-info__moreinfo' data-title={this.props.title}>Add to watch list</button>
: <button id='' onClick={(e) => this.handleWatchList(e, "deleteFromWatchList")} className='movie-info__moreinfo' data-title={this.props.title}>Remove from watch list</button>}
<button onClick={(e) => this.movieFetch(e)} id='movie-info__moreinfo' className='movie-info__moreinfo'>Info</button>
</div>
</div>
);
};
}

export default Movie;
42 changes: 42 additions & 0 deletions src/Movies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import React from 'react';
import Movie from './Movie';


class Movies extends React.Component {
constructor() {
super();
this.watchListReceiver = this.watchListReceiver.bind(this);
}

watchListReceiver(title, action) {
this.props.receiver(title, action);
}

render() {

console.log(this.props.movies.length);
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid committing console.logs

console.log(this.props.movies);
return (
this.props.movies.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.

Avoid having multi-line ternaries in JSX. I would use an if/else and return from each block.

?
<section className="search-results" id="search-results">
<div className='movie-list-wrapper'>
{this.props.movies.map((movie) => {
return <Movie
watchlistbutton={this.props.watchlistbutton}
key={movie.imdbID}
poster={movie.Poster}
title={movie.Title}
year={movie.Year}
imdbID={movie.imdbID}
receiver={this.watchListReceiver} />;
})}
</div>
</section>
:
""
);
}
}

export default Movies;
Loading