React Cinema - Ralph #5
Conversation
| search: [], | ||
| searchTerm: "", | ||
| page: 2, | ||
| watchList: localStorage.getItem("watchList") ? localStorage.getItem("watchList") : [], |
There was a problem hiding this comment.
You may need to perform JSON.parse on item retrieved from localStorage otherwise I expect it will just be a string
| const query = 'http:https://www.omdbapi.com/?s=' + this.state.searchTerm + '&apikey=' + OMBbAPIKey + '&page=' + this.state.page; | ||
|
|
||
| // Back to top button | ||
| if (body.scrollTop > 300 || html.scrollTop > 300) { |
There was a problem hiding this comment.
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
| if (windowBottom >= docHeight) { | ||
| fetch(query) | ||
| .then(response => { | ||
| loading.textContent = "Loading..."; |
There was a problem hiding this comment.
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
| if (movies.Search) { | ||
| const self = this; | ||
| setTimeout(function () { | ||
| self.setState({ |
There was a problem hiding this comment.
You can avoid using self by using an arrow function. It will set the value of this to what it is in parent scope.
| } | ||
|
|
||
| componentDidMount() { | ||
| window.addEventListener("scroll", this.handleScroll); |
| } | ||
|
|
||
| receiverWatchList(articleTitle, action) { | ||
| if (action === "addToWatchList") { |
There was a problem hiding this comment.
I would suggest implementing this as two functions. One will do addToWatchList and another deleteFromWatchList
There was a problem hiding this comment.
Keeping function to a single purpose will keep manageable and prevent them from becoming too large and doing too many things
| watchList: [...this.state.watchList, articleObj] | ||
| }) | ||
|
|
||
| localStorage.getItem("watchList") |
There was a problem hiding this comment.
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));
| render(){ | ||
| render() { | ||
| const watchList = this.state.watchListSection; | ||
| let MoviesList; |
There was a problem hiding this comment.
avoid using capitals for things which are not classes or component names
There was a problem hiding this comment.
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();
| this.handleWatchList = this.handleWatchList.bind(this); | ||
| } | ||
| handleWatchList() { | ||
| document.querySelectorAll('.movie-info__moreinfo').forEach(item => item.classList.remove("disabled")); |
There was a problem hiding this comment.
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
|
|
||
| render() { | ||
|
|
||
| console.log(this.props.movies.length); |
There was a problem hiding this comment.
avoid committing console.logs
| console.log(this.props.movies.length); | ||
| console.log(this.props.movies); | ||
| return ( | ||
| this.props.movies.length > 0 |
There was a problem hiding this comment.
Avoid having multi-line ternaries in JSX. I would use an if/else and return from each block.
| {/* <ul id="autocomplete" className="autocomplete"> | ||
| </ul> */} | ||
| <button className="movie-request__submit" type="submit">Search</button> | ||
| {/* <button className="movie-request__filters-toggle" id="movie-request__filters-toggle" type="button">Search filters</button> |
There was a problem hiding this comment.
Avoid committing commented out code
| event.preventDefault(); | ||
|
|
||
| this.setState({ | ||
| search: event.target.value, |
There was a problem hiding this comment.
what's the difference between search and searchTerm?
| @import url("https://fonts.googleapis.com/css?family=Source+Sans+Pro"); | ||
|
|
||
| :root { | ||
| --primary-color: #f1f1f1; |
| } | ||
|
|
||
| @media (min-width: 480px) { | ||
| .movie-poster img { |
|
Some great functionality. I would be nice to create a little helper library to handle all the localStorage behaviour in one place. Also, mixing DOM manipulation and React is a bad idea, it can cause weird bugs which can be tricky to fix. Let me know if you want help replacing DOM manipulation with React behaviour. |
No description provided.