Conversation
JennieDalgren
left a comment
JennieDalgren
left a comment
There was a problem hiding this comment.
Great job with this project!
There are some things we want you to fix in order to get approved on this project.
In the HTML file you have hard-coded all the movies in the movies section. This should be empty, and populated with the help of javascript, like you do with the function called displayAllMovies
Nice job with having an empty state - if no movies in a specific genre is found you are showing
if (filteredMovies.length === 0) {
document.getElementById('movies-section').innerHTML = '<p>No movies found for the selected genre.</p>';
} else {
displayMovies(filteredMovies);
}
Why are you using the spread operator? [...movies]
Sometimes you are putting the functions inline, in the eventlistener and sometimes you break them out. Stick to one approach.
Either like this:
document.getElementById('romance').addEventListener('click', () => {
if (currentGenre === 'romance') {
currentGenre = '';
displayAllMovies();
} else {
currentGenre = 'romance';
recommendMovieByGenre(currentGenre);
}
});
or like this:
// Add event listeners for the sorting buttons
document.getElementById('sort-new-to-old').addEventListener('click', sortMoviesNewToOld);
Another suggestion is to add some comments to you code, specially when you re working in a group or pair. So that you all remember why you have coded the way you have etc.
TODO:
- Clean up the HTML and let JS take care of populating the movies section.
- Refactor the event listeners to use the same approach everywhere.
Keep it up! 💪
cholpoun
left a comment
cholpoun
left a comment
There was a problem hiding this comment.
Good job! Wishing all the best future projects!
| // Function to filter and display movies based on genre | ||
| function recommendMovieByGenre(genre) { | ||
| const filteredMovies = movies.filter(movie => { | ||
| const genres = movie.genre.split(',').map(g => g.trim().toLowerCase()); |
There was a problem hiding this comment.
Suggestion to have a button to be able to see all the movies in the array /Kasia, Cholpon, Nella
index.html
Outdated
| <img src="./movie-images/inception-movie.jpg" alt="Inception" /> | ||
| <div class="card-content"> | ||
| <h2>Inception</h2> | ||
| <p><strong>Director:</strong> Christopher Nolan | <strong>Year:</strong> 2010</p> |
There was a problem hiding this comment.
Interesting! We have never tried ! /Kasia, Cholpon, Nella
| of the properties make sense to filter on, and some to sort on.*/ | ||
|
|
||
| const books = [ | ||
| const movies = [ |
There was a problem hiding this comment.
Great job! Good descriptions and easy to follow, well structured! /Kasia, Cholpon, Nella
| color: white; | ||
| } | ||
|
|
||
| @media (min-width:900px) { |
There was a problem hiding this comment.
In iPhone 5/SE, the text doesn't fit inside the card. You can change the flex-direction to a column, and then image will go on top and the text under. /Kasia, Cholpon, Nella
| font-family: 'Gill Sans', 'Gill Sans MT', Calibri, 'Trebuchet MS', sans-serif; | ||
| } | ||
|
|
||
| .hero { |
There was a problem hiding this comment.
Good job with the hero image and text being responsive in different screen sizes. /Kasia, Cholpon, Nella
3 participants
https://filmnestmovies.netlify.app/
https://filmnestmovies.netlify.app/
Collaborators
https://github.com/smily342 and https://github.com/Heleneabrahamsson