React Cinema - Ralph #5

r41ph · 2018-06-27T09:24:09Z

No description provided.

dmitrigrabov · 2018-07-01T19:31:07Z

src/App.js

+      search: [],
+      searchTerm: "",
+      page: 2,
+      watchList: localStorage.getItem("watchList") ? localStorage.getItem("watchList") : [],


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

dmitrigrabov · 2018-07-01T19:32:51Z

src/App.js

+    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) {


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

dmitrigrabov · 2018-07-01T19:34:47Z

src/App.js

+    if (windowBottom >= docHeight) {
+      fetch(query)
+        .then(response => {
+          loading.textContent = "Loading...";


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

dmitrigrabov · 2018-07-01T19:36:04Z

src/App.js

+          if (movies.Search) {
+            const self = this;
+            setTimeout(function () {
+              self.setState({


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

dmitrigrabov · 2018-07-01T19:36:23Z

src/App.js

+  }
+
+  componentDidMount() {
+    window.addEventListener("scroll", this.handleScroll);


dmitrigrabov · 2018-07-01T19:39:16Z

src/App.js

+  }
+
+  receiverWatchList(articleTitle, action) {
+    if (action === "addToWatchList") {


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

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

dmitrigrabov · 2018-07-01T19:46:08Z

src/App.js

+              watchList: [...this.state.watchList, articleObj]
+            })
+
+            localStorage.getItem("watchList")


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));

dmitrigrabov · 2018-07-01T19:47:32Z

src/App.js

-  render(){
+  render() {
+    const watchList = this.state.watchListSection;
+    let MoviesList;


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

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();

dmitrigrabov · 2018-07-01T19:50:14Z

src/Header.js

+        this.handleWatchList = this.handleWatchList.bind(this);
+    }
+    handleWatchList() {
+        document.querySelectorAll('.movie-info__moreinfo').forEach(item => item.classList.remove("disabled"));


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

dmitrigrabov · 2018-07-01T20:02:12Z

src/Movies.js

+
+    render() {
+
+        console.log(this.props.movies.length);


avoid committing console.logs

dmitrigrabov · 2018-07-01T20:04:47Z

src/Movies.js

+        console.log(this.props.movies.length);
+        console.log(this.props.movies);
+        return (
+            this.props.movies.length > 0


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

dmitrigrabov · 2018-07-01T20:05:22Z

src/Search.js

+                    {/* <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>


Avoid committing commented out code

dmitrigrabov · 2018-07-01T20:05:52Z

src/Search.js

+        event.preventDefault();
+
+        this.setState({
+            search: event.target.value,


what's the difference between search and searchTerm?

dmitrigrabov · 2018-07-01T20:06:23Z

src/css/style.css

+@import url("https://fonts.googleapis.com/css?family=Source+Sans+Pro");
+
+:root {
+  --primary-color: #f1f1f1;


dmitrigrabov · 2018-07-01T20:06:55Z

src/css/style.css

+}
+
+@media (min-width: 480px) {
+  .movie-poster img {


dmitrigrabov · 2018-07-01T20:08:55Z

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.

Ralph added 10 commits June 22, 2018 20:23

Added search

2792f13

Added infinite scroll

6a6db11

Update style movies list to two columns

4be3665

Added Back To Top button

01b1b79

Refactoring Loading and BackToTop into components

05becbb

Added Info button

d7f4544

Bug fix placeholder image

f2f47ad

Added watch list

5a4b372

Added watchList to localStorage

fe92891

Working on localStorage and watchList

a151f78

dmitrigrabov reviewed Jul 1, 2018

View reviewed changes

src/App.js

}

componentDidMount() {

window.addEventListener("scroll", this.handleScroll);

Copy link

Contributor

dmitrigrabov Jul 1, 2018

Choose a reason for hiding this comment

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

nice

dmitrigrabov reviewed Jul 1, 2018

View reviewed changes

src/Movies.js

render() {

console.log(this.props.movies.length);

Copy link

Contributor

dmitrigrabov Jul 1, 2018

Choose a reason for hiding this comment

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

avoid committing console.logs

dmitrigrabov reviewed Jul 1, 2018

View reviewed changes

src/css/style.css

}

@media (min-width: 480px) {

.movie-poster img {

Copy link

Contributor

dmitrigrabov Jul 1, 2018

Choose a reason for hiding this comment

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

nice

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React Cinema - Ralph #5

React Cinema - Ralph #5

r41ph commented Jun 27, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018 •

edited

Loading

dmitrigrabov Jul 1, 2018

dmitrigrabov Jul 1, 2018

dmitrigrabov commented Jul 1, 2018

React Cinema - Ralph #5

Are you sure you want to change the base?

React Cinema - Ralph #5

Conversation

r41ph commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrigrabov Jul 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrigrabov commented Jul 1, 2018

dmitrigrabov Jul 1, 2018 •

edited

Loading