The Reel Thing #14
The Reel Thing #14
Conversation
src/components/App.js
Outdated
| this.setState({ | ||
| filmDetails: body, | ||
| hints: [] | ||
| }, () => this.toggleVisible()) |
There was a problem hiding this comment.
I would probably use isClosed: !this.state.isClosed in setState to avoid having to use a callback
src/components/App.js
Outdated
| query: query, | ||
| currentPage: 1, | ||
| filmDetails: {} | ||
| }, () => this.searchByTitle(query, this.state.currentPage)); |
There was a problem hiding this comment.
I would take the search function out of the callback and pass in a hardcoded page number. Since searchByTitle only uses params and not state inside it, it should be fine
src/components/App.js
Outdated
| receiveSearchHint(string) { | ||
| this.setState({ | ||
| query: string, | ||
| }, () => this.searchHint(this.state.query)); |
There was a problem hiding this comment.
As above, I would take searchHint out of the callback and use this.searchHint(string). It avoids having to use the callback
src/components/App.js
Outdated
| }, () => this.searchByTitle(this.state.query, this.state.currentPage)); | ||
| } | ||
|
|
||
| receiveFavListState(boolean) { |
There was a problem hiding this comment.
boolean is a bit ambiguous. I would give the param a more specific name
src/components/App.js
Outdated
| favourites.unshift(obj); | ||
| this.setState({ | ||
| favourites: favourites | ||
| }, () => window.localStorage.setItem("favourites", JSON.stringify(this.state.favourites))); |
There was a problem hiding this comment.
I would take window.localStorage.setItem("favourites", JSON.stringify(this.state.favourites)) out of callback and use window.localStorage.setItem("favourites", JSON.stringify(favourites))
src/components/App.js
Outdated
| favourites.splice(index, 1); | ||
| this.setState({ | ||
| favourites: favourites | ||
| }, () => window.localStorage.setItem("favourites", JSON.stringify(this.state.favourites))); |
| this.handleClick = this.handleClick.bind(this); | ||
| } | ||
|
|
||
| handleClick(e) { |
There was a problem hiding this comment.
I would probably create two separate click handlers, one for each action rather than checking classes. It would make the code a tiny bit easier to read
| handleClick(e) { | ||
| this.setState({ | ||
| showFavs: !this.state.showFavs | ||
| }, () => this.props.receiveFavListState(this.state.showFavs)) |
There was a problem hiding this comment.
I would take the receiver call out of callback and pass to it a pre-calcualated value
src/components/Pagination.js
Outdated
| handleClick(e) { | ||
| const totalPages = Math.ceil(this.props.totalFilms / this.props.filmsPerPage) | ||
|
|
||
| if (e.target.className.includes("btn__next") && this.state.currentPage < totalPages) { |
There was a problem hiding this comment.
As before, I would suggest separate click handlers and not using setState callbacks
| } | ||
|
|
||
| handleClick(e) { | ||
| this.props.receiveFilmID(e.target.parentNode.dataset.id); |
There was a problem hiding this comment.
It's better practice not use data attributes in React and instead work with data from props
|
Good work, but I would recommend avoiding setState callbacks where possible. Also, using more click handlers will save having to write logic which checks classes |
2 participants
I seem to be passing a lot of functions and states around, and I feel that it could be more straight-forward.
Especially with toggling styles and getting the search form to minimise only on the first user interaction.