-
Notifications
You must be signed in to change notification settings - Fork 198
[Flixster] Movie Explorer Project #63
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
base: main
Are you sure you want to change the base?
Conversation
… Movie List, and Footer done. Movie Cards should be responsive to window size and are being updated dynamically by data file
Added Search and Sort features
…buttons and updating displayed movies using sidebar
…calls to only new data. searchTrigger state is a band aid fix for now until I can find a better way
Adding Final stretch features and cleaning up bugs
… way like and watched info is stored
Merge branches#
src/Header.jsx
Outdated
| function Header({search,clear,searchTermFunction,searchString, sortFunc}) { | ||
| function Header({search,clear,searchTermFunction,searchString, sortFunc, display}) { | ||
|
|
||
| console.log(display) |
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.
remove console logs :-)
| <h1>Flixster</h1> | ||
| <SearchForm searchTerm={searchString} searchFunction={search} clearFunction={clear} searchTermFunc={searchTermFunction} sortFunc={sortFunc}/> | ||
| <SearchForm display={display} searchTerm={searchString} | ||
| searchFunction={search} clearFunction={clear} |
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.
I think you can improve on naming the props, in most cases I've seen that the callbacks/functions you pass may be easier to interpet by using naming like onSearch vs searchFunction & onClear vs clearFunction
in short drop the Function add the on
| }else { | ||
| return ( | ||
| <div className="no-movies-splash"> | ||
| <h2 className="splash-text">No movies here...</h2> | ||
| <h3 className="splash-icon">{'\u{2639}'}</h3> | ||
| </div> | ||
| ); | ||
| } |
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.
As If condition is already returning, we don't need specific else here. You can do something like:
if(condition){
return <...>;
}
return <...> // this will work as else condition without explicit else :)
| if (type === 'alpha'){ | ||
| return sortedArr.sort((a,b) => a.title.localeCompare(b.title)); | ||
| }else if (type === 'release'){ | ||
| return sortedArr.sort((a,b) => b.release_date.localeCompare(a.release_date)) | ||
| }else if (type === 'vote'){ | ||
| return sortedArr.sort((a,b) => b.vote_average - a.vote_average) | ||
| }else { | ||
| return arr | ||
| } |
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.
How about updating this into a switch case instead of long if..else..if... conditions?
src/App.jsx
Outdated
| } | ||
|
|
||
| const updateFavs = (movie,faved) => { | ||
| faved ? setFavMovies([...favMovies, movie]) : setFavMovies(favMovies.filter(element => element !== movie)); |
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.
nit: Maybe try to generate the object to be set and call setFavMovies only once
const toggleFavs = (movie,faved) => {
const favs = faved ? [...favMovies, movie] : favMovies.filter(element => element !== movie);
setFavMovies(favs);
src/App.jsx
Outdated
| <Header clear={clearSearch} search={search} searchTermFunction={updateSearchTerm} searchString={searchString} sortFunc={updateSortType}/> | ||
| <SideNav homeFunc={openHome} favFunc={openFavorites} watchFunc={openWatched}/> | ||
| <Body data={page !== 'Home' ? (page === 'Favorite' ? favMovies : watchedMovies) : movieData} load={load}/> | ||
| <Body data={page !== 'Home' ? (page === 'Favorites' ? favMovies : watchedMovies) : movieData} load={load} |
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.
I think the load name is not clear on what it does, maybe consider using pageNumber as prop name to make it easy to read
src/components/MovieButtons.jsx
Outdated
| const [faved, setFaved] = useState(false); | ||
| const [watched, setWatched] = useState(false) | ||
|
|
||
| useEffect(() => { |
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.
you may not need these useEffect functions you can simplify this by simply passing the addToFav & addToWatched callbacks directly to the onClick prop example
const handleFavClick = (evt) => {
evt.stopPropagation();
setFaved(!faved);
addToFav(movie, !faved);
}
<span className='favButton' onClick={handleFavClick}>{faved? '\u{1F496}':'\u{1F90D}'}</span>| <h3><strong>{title}</strong></h3> | ||
| <p>Rating: {rating}</p> | ||
| <MovieButtons /> | ||
| <img src={imgURL + posterSize + movie.poster_path} alt={movie.title + " poster"} /> |
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.
This is fine, but I'd suggest you try to use URL js builder just to make sure the URL is well formed https://developer.mozilla.org/en-US/docs/Web/API/URL/URL
// Base URLs:
let baseUrl = "https://developer.mozilla.org";
let A = new URL("/", baseUrl);
// => 'https://developer.mozilla.org/'
let B = new URL(baseUrl);
// => 'https://developer.mozilla.org/'
new URL("en-US/docs", B);
// => 'https://developer.mozilla.org/en-US/docs'
let D = new URL("/en-US/docs", B);
// => 'https://developer.mozilla.org/en-US/docs'
new URL("/en-US/docs", D);
// => 'https://developer.mozilla.org/en-US/docs'
new URL("/en-US/docs", A);
// => 'https://developer.mozilla.org/en-US/docs'
new URL("/en-US/docs", "https://developer.mozilla.org/fr-FR/toto");
// => 'https://developer.mozilla.org/en-US/docs'
|
|
||
| useEffect(() => { | ||
| const fetchMovieData = async () => { | ||
| if(movie.id === undefined){ |
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.
unfortunately in JS world you want to check for undefined & null at the same time, try using
movie.id == null| const openHome = () => { | ||
| setPage('Home') | ||
| } | ||
|
|
||
| const openFavorites = () => { | ||
| setPage('Favorites') | ||
| } | ||
|
|
||
| const openWatched = () => { | ||
| setPage('Watched') | ||
| } |
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.
This looks repetitive, when you see something repetitive it means you can DRY (Dont Repeat Yourself)
To transform wetcode to DRY code you can do something like
const openPage(pageName) {
setPage(pageName);
}
src/components/MovieButtons.jsx
Outdated
| export default function MovieButtons() { | ||
|
|
||
| const handleClick = evt => { | ||
| evt.stopPropigation(); |
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.
is this intentionally blank?
| var res1 = await fetch(url, options) | ||
| var res2 = await fetch(videoUrl, options) |
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.
Try to use const instead of var keyword
also you can group promises in a single call like
const [moveies, videos] = await Promise.all([fetch(url, options), fetch(videoUrl, options)])| const App = () => { | ||
| //States to store the current movie data | ||
| const [movieData, setMovieData] = useState([]) | ||
| const [pageNum, setPageNum] = useState(1) |
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.
Controversial, arrays start at 0 D=
| const [searchString, setSearchString] = useState('') | ||
|
|
||
| //URL for featching data from API | ||
| const nowPlayingURL = `https://api.themoviedb.org/3/movie/now_playing?language=en-US&page=${pageNum}` |
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.
Also consider using a URL builder if possible
src/App.jsx
Outdated
| } | ||
| } | ||
| fetchMovieData(); | ||
| },[url]) |
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.
it feels odd that we are relying on the url for refetching movie data based on that, I'd recommend
- generating the url inside the fetchMovie data function
- create a function outside the component scope that encapsulates the fetching logic
example:
const FETCH_OPTIONS = {method: 'GET', headers: {accept: 'application/json',
Authorization: `Bearer ${API_KEY}`}};
async function fetchMovies(page) {
const url = `https://api.themoviedb.org/3/movie/now_playing?language=en-US&page=${pageNum ?? 0}`;
try{
var res = await fetch(url, FETCH_OPTIONS)
if(res.ok){
const data = await res.json();
return {movies: data.results, error: null };
}else{
return {error: "Error Message"};
}
}catch(error){
return {error: error.message};
}
}
src/App.jsx
Outdated
|
|
||
| //Update the url for loading more movies | ||
| useEffect(() => { | ||
| setUrl(nowPlayingURL) |
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.
if we drop the fetching dependency on url, you may no longer need this
src/App.jsx
Outdated
|
|
||
| useEffect(() => { | ||
|
|
||
| console.log('do sort') |
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.
clean up console logs :-)
| },[sortType]) | ||
|
|
||
|
|
||
| const updateSortType = evt => { |
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.
you may not need this + the useEffect consider having the update setter to be part of the click handler
const updateSortType = (evt) => {
setSortType(evt.target.value);
const sortedMovies = sort(movieData,sortType)
setMovieData(sortedMovies)
}
src/App.jsx
Outdated
| fetchMovieData(); | ||
| },[pageNum]) | ||
|
|
||
| const load = () => setPageNum(pageNum + 1) |
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.
consider renaming this to loadNext or just nextPage to make it clear
|
|
||
| //API Info for fetch | ||
| const API_KEY = import.meta.env.VITE_API_KEY | ||
| const options = {method: 'GET', headers: {accept: 'application/json', |
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.
Looking at this I think you can try to generate utils for fetching
// Movies.api.js
export async function fetchMovies() {
// fetch logic here
return {movies, error};
}
// App.jsx
const {movies, error} = await fetchMovies(page);
if (error) {
setErrorMsg(error);
}
...
.env
Outdated
| @@ -1 +1 @@ | |||
| VITE_API_KEY=eyJhbGciOiJIUzI1NiJ9.eyJhdWQiOiIwZmJhOGUxMThkYjlkYjQ1MDViZjJlNDIxOTA2YjBiMCIsIm5iZiI6MTc0OTUwOTQ1OC41NDU5OTk4LCJzdWIiOiI2ODQ3NjU1MmU5ODg5ZGMzMzMyMDY4OTYiLCJzY29wZXMiOlsiYXBpX3JlYWQiXSwidmVyc2lvbiI6MX0.hC15ggnHAhw1hfT_FHTOmwbKnG9c7qFyvMJeSw7LbHs'}}; | |||
| VITE_API_KEY=eyJhbGciOiJIUzI1NiJ9.eyJhdWQiOiIwZmJhOGUxMThkYjlkYjQ1MDViZjJlNDIxOTA2YjBiMCIsIm5iZiI6MTc0OTUwOTQ1OC41NDU5OTk4LCJzdWIiOiI2ODQ3NjU1MmU5ODg5ZGMzMzMyMDY4OTYiLCJzY29wZXMiOlsiYXBpX3JlYWQiXSwidmVyc2lvbiI6MX0.hC15ggnHAhw1hfT_FHTOmwbKnG9c7qFyvMJeSw7LbHs No newline at end of file | |||
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.
🙈
| return ret.slice(0, -2) | ||
| } | ||
|
|
||
| if(movieDetails === null){ |
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.
what if the movieDetails are empty?
| function genreString(genres) { | ||
| let ret = '' | ||
| for(var i = 0; i < genres.length; ++i){ | ||
| ret += (genres[i].name + ', ') |
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.
Let me introduce you the join function https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/join
| const [show, setShow] = useState(false); | ||
|
|
||
| const close = () => setShow(false); | ||
| const open = id => event => { |
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.
Not sure I follow what the id is doing for us, we are always returning the same function
Flixster
Main Features
Display Movies in Grid View-

-Using CSS flexbox I created the container for the list of movie cards
Users can load more movies using "Load More" button-

-TMDB API allows you to make requests for movies by page numbers, by pressing 'load more' the page number is incremented and an additional fetch request is made.
Search Bar which allows users to search TMDB database-

-TMDB API has an additional API call for searching movies based on a given substring, I use this to make a new fetch request and store the searched data in a new container. Following this I update the currently displayed movie data. The user can click the 'enter' key or submit to search, and they can also click 'clear' to go back to the original data.
Movie details displayed when movie card is clicked-

-Each movie card has an on click listener that opens a modal upon click. The modal is repopulated each time a new movie is clicked with the new movie details. These details require an additional fetch request from the API.
Sort currently displayed movies using sort button-

-Use a sort function in the utils folder to sort the movieData state (currently displayed data) and then update the display based on the sort. 3 different methods of sorting, alphabetical, release date, and rating.
Website should implement responsive web design-
Stretch Features
Embedded Video Trailers-

-Must make an additional fetch request when accessing the movie details modal (this sometimes causes the old video to still display for a second). Used an iframe component to implement the feature.
Favorite & Watched buttons

-Simple button components, added some basic animations. I also created 2 additional containers for movies, a favMovies and watchedMovies, whenever one of the buttons is clicked it adds a copy of that movie data into the correct container.
Sidebar, switches to showing only favorited or watched movies-

-Switches between what container is being shown, clicking on the star sets the favMovies container as our data, while clicking on the flag sets the watchedMovies container as our data. Also when switching to these pages I hide the search, sort, and load more features as it doesn't make sense to show them.