-
Notifications
You must be signed in to change notification settings - Fork 84
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
Code Review #27
base: main
Are you sure you want to change the base?
Code Review #27
Changes from 3 commits
6bebcf1
5d03bde
065f2a2
d63990f
e5d78ad
026f9f2
883bfe1
7916f26
c3d439a
091f138
7472cab
147c02e
2f3a39b
c6f4bf5
97585ba
c9ad1e3
116414d
677bb79
e7fe6ff
80ebbb4
04a9369
d7d123c
ef5479d
0ef6526
58adaa7
7b94bce
6247b27
e7f81f1
7d53211
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,78 @@ | ||
.App { | ||
#root { | ||
max-width: 1900px; | ||
margin: 0 auto; | ||
/* padding: 2rem; */ | ||
text-align: center; | ||
background-color: #f5f5f5; | ||
} | ||
|
||
.App-header { | ||
background-color: #282c34; | ||
.header { | ||
border: 2px solid indigo; | ||
width: 100%; | ||
background-color: indigo; | ||
display: fixed; | ||
} | ||
|
||
.title { | ||
color: white; | ||
} | ||
form{ | ||
display: flex; | ||
flex-direction: row; | ||
align-items: center; | ||
justify-content: space-evenly; | ||
justify-content: flex-start; | ||
margin-left: 25px; | ||
} | ||
|
||
#search-bar { | ||
padding: 10px; | ||
width: 500px; | ||
margin-right: 3px; | ||
|
||
} | ||
|
||
#search-button { | ||
background-color: #B19CD9; | ||
padding: 10px; | ||
border: 2px solid darkmagenta; | ||
border-radius: 5px; | ||
} | ||
|
||
#search-button:hover, | ||
#nowPlayingButton:hover, | ||
#searchTabButton:hover { | ||
color: white; | ||
padding: 20px; | ||
background-color: darkmagenta; | ||
border: 2px solid #B19CD9; | ||
border-radius: 5px; | ||
} | ||
|
||
@media (max-width: 600px) { | ||
.movie-card { | ||
width: 100%; | ||
} | ||
#nowPlayingButton { | ||
background-color: #B19CD9; | ||
padding: 10px; | ||
border: 2px solid darkmagenta; | ||
border-radius: 5px; | ||
margin-right: 10px; | ||
} | ||
|
||
#searchTabButton { | ||
background-color: #B19CD9; | ||
padding: 10px; | ||
border: 2px solid darkmagenta; | ||
border-radius: 5px; | ||
} | ||
|
||
.search-bar { | ||
flex-direction: column; | ||
gap: 10px; | ||
|
||
.searchForm { | ||
display: none; | ||
} | ||
|
||
@media all and (max-width: 630px) { | ||
#search-bar { | ||
width: 300px; | ||
} | ||
} | ||
|
||
.search-bar form { | ||
flex-direction: column; | ||
@media all and (max-width: 400px) { | ||
#search-bar { | ||
width: 100px; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,106 @@ | ||
import { useState } from 'react' | ||
import './App.css' | ||
import React from 'react'; | ||
import MovieList from './MovieList'; | ||
import Search from './Search'; | ||
import { useEffect, useState } from 'react'; | ||
import Modal from './Modal'; | ||
import './App.css'; | ||
import './Modal.css'; | ||
|
||
const App = () => { | ||
<div className="App"> | ||
|
||
</div> | ||
function App() { | ||
const [movies, setMovies] = useState([]); | ||
const [page, setPage] = useState(1); | ||
const [searchValue, setSearchValue] = useState(''); | ||
const [modal, setModal] = useState(false); | ||
const [movieID, setMovieID] = useState(''); | ||
|
||
const getMovies = async () => { | ||
const url = `https://api.themoviedb.org/3/movie/now_playing?language=en-US&page=${page}&api_key=0d7613c1b95dbc61f3dd491c8f802475`; | ||
const options = { | ||
method: 'GET', | ||
headers: { | ||
accept: 'application/json', | ||
Authorization: 'Bearer eyJhbGciOiJIUzI1NiJ9.eyJhdWQiOiIwZDc2MTNjMWI5NWRiYzYxZjNkZDQ5MWM4ZjgwMjQ3NSIsInN1YiI6IjY2Njc2NmJhODAyN2M0OWNmYjk5ZmJiYyIsInNjb3BlcyI6WyJhcGlfcmVhZCJdLCJ2ZXJzaW9uIjoxfQ.0iAwLYCfoeheb8A8_TWhHNrdDn2P1x3LL-d0_fGs6BA' | ||
} | ||
}; | ||
|
||
const response = await fetch(url, options); | ||
const jsonResponse = await response.json(); | ||
if (page === 1){ | ||
setMovies(jsonResponse.results) | ||
} else { | ||
setMovies(prevMovies => [...prevMovies, ...jsonResponse.results]) | ||
} | ||
|
||
} | ||
|
||
const searchMovies = async () => { | ||
const result = searchValue.replace(/ +/g, '+'); | ||
const url = `https://api.themoviedb.org/3/search/movie?query=${result}&api_key=0d7613c1b95dbc61f3dd491c8f802475`; | ||
const options = { | ||
method: 'GET', | ||
headers: { | ||
accept: 'application/json', | ||
Authorization: 'Bearer eyJhbGciOiJIUzI1NiJ9.eyJhdWQiOiIwZDc2MTNjMWI5NWRiYzYxZjNkZDQ5MWM4ZjgwMjQ3NSIsInN1YiI6IjY2Njc2NmJhODAyN2M0OWNmYjk5ZmJiYyIsInNjb3BlcyI6WyJhcGlfcmVhZCJdLCJ2ZXJzaW9uIjoxfQ.0iAwLYCfoeheb8A8_TWhHNrdDn2P1x3LL-d0_fGs6BA' | ||
} | ||
}; | ||
const response = await fetch(url, options); | ||
const jsonResponse = await response.json(); | ||
const results = jsonResponse.results.filter((movie) => { | ||
return movie.title && movie.title.toLowerCase().includes(searchValue.toLowerCase()); | ||
}) | ||
if (results.length === 0){ | ||
return "Try again, No results found :)" | ||
}else { | ||
setMovies(results); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall looks good to me! The website looks great to me and functioning well! Just a few nit on spacing and newlines. I noticed that a lot of functions have series of consecutive newlines, can we delete the newlines to make file structure cleaner? |
||
|
||
} | ||
|
||
useEffect(() => { | ||
getMovies(); | ||
}, [page]) | ||
|
||
useEffect(() => { | ||
searchMovies() | ||
}, [searchValue]) | ||
|
||
const loadMore = () => { | ||
setPage(page => page + 1); | ||
}; | ||
// setModal(!modal); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this line? |
||
const movieClick = (movieId) => { | ||
setMovieID(movieId); | ||
} | ||
|
||
|
||
return ( | ||
<> | ||
<header className="header"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should have an indent. Somehow format is important for code readebility. things like indentation, empty line,... |
||
<h1 className="title">Flixster</h1> | ||
<Search className="searchForm" searchValue={searchValue} setSearchValue={setSearchValue}/> | ||
|
||
<button id="nowPlayingButton" onClick={() => { | ||
const form = document.getElementsByClassName("searchForm")[0]; | ||
const loadButton = document.getElementById('load-more'); | ||
form.style.display = 'none'; | ||
loadButton.style.display = 'grid'; | ||
loadButton.style.marginLeft = '40%'; | ||
getMovies(); | ||
}}>Now Playing</button> | ||
|
||
<button id="searchTabButton" onClick={() => { | ||
const form = document.getElementsByClassName("searchForm")[0]; | ||
const loadButton = document.getElementById('load-more'); | ||
form.style.display = 'block'; | ||
loadButton.style.display = 'none'; | ||
searchMovies(); | ||
}}>Search</button> | ||
</header> | ||
<Modal data={movies} movieID={movieID}/> | ||
<MovieList data={movies} loadMore={loadMore} handleMovieClick={movieClick}/> | ||
</> | ||
) | ||
} | ||
|
||
export default App | ||
export default App; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can do |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
.movieModal{ | ||
display: none; | ||
position: fixed; | ||
z-index: 1; | ||
left: 0; | ||
top: 0; | ||
width: 100%; | ||
height: 100%; | ||
overflow: hidden; | ||
background-color: rgba(225, 225, 225, 0.5); | ||
/* margin-top: 15px; */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this unused line? |
||
} | ||
|
||
.modalContent { | ||
display: grid; | ||
position: relative; | ||
margin: 10% auto; | ||
padding: 20px; | ||
width: 63%; | ||
height: 75%; | ||
border: 2px solid black; | ||
border-radius: 10px; | ||
background-color: black; | ||
justify-content: center; | ||
color: white; | ||
} | ||
|
||
.body-no-scroll { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's try to keep the styles name format consistent, |
||
overflow: hidden; | ||
height: 100%; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we reuse
background-color: #B19CD9;
quite a few times here, in the future let's try to capture repeated styles in a separate class so we can avoid repetition