PR#32
Conversation
…he bottom of the page
|
Forgot to add Netlify-link: https://recipeproject-sofia.netlify.app/ |
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Nice and descriptive function names, continue with this! Remember to always stick to const as much as you can. Basically you only need to use let if you bump into issues. Also, you have some repetitive code that could be cleaned up (e.g. the duplicate URL) as well as some console.logs that should not be kept in production code.
Some other styling ideas:
- There’s no special styling on the chosen diet filter, just on the chosen cuisine. Is this deliberate?
- You’ve chosen to put the instructions directly in the card (instead of just showing them on a button click or something), which isn’t wrong or anything, but since some instructions are very long it makes the card grid look a bit off. Consider showing only part of the instructions and then showing the rest on a button click or hover (i.e. Show more… button).
Changes requested
- Can’t see you’ve met this requirement: “Your page should show a useful message to the user in case the daily API quota has been reached”
- Your page is not fully responsive. Please have a look at how your app looks in mobile (320px width). It’s a bit too crowded 👀
- The user should be able to click a button that selects a random recipe, and your page should display the selected item
- Your page should have an empty state, e.g. if there are no recipes matching the filter - show a meaningful message to the user
…added a message section that displays errors/when the api quota is reached, improved the random recipe button
|
Hey @HIPPIEKICK ! Thank you for your feedback. I've tried to make the requested changes, please have a look and see what you think :) |
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Sorry, clicked the wrong button 😅 You're approved!
|
Thank you so much @HIPPIEKICK ! |
mimmi-eriksson
left a comment
mimmi-eriksson
left a comment
There was a problem hiding this comment.
Overall, the site looks and works great! Nice work with the styling and the filters, sorting and random functions all works as they should. Your code is really clear and structured which makes it very easy to read! I just added some minor comments and suggestions.
Keep up the good work Sofia!
There was a problem hiding this comment.
Clear and nicely structured html!
|
|
||
| <section | ||
| id="messages" | ||
| style="display: none;" |
There was a problem hiding this comment.
This line could be removed since you have it in the css :)
There was a problem hiding this comment.
Nice and clear, easy to read code! Naming of variables and functions are very descriptive👍
| let currentPage = 1; // Håller reda på vilken sida vi är på | ||
| const recipesPerPage = 20; // Hur många recept vi vill hämta per gång | ||
| let scrollEnabled = true; // Håller reda på om scroll är aktiverad |
There was a problem hiding this comment.
You might want to change the comments to english
| let currentPage = 1; // Håller reda på vilken sida vi är på | ||
| const recipesPerPage = 20; // Hur många recept vi vill hämta per gång | ||
| let scrollEnabled = true; // Håller reda på om scroll är aktiverad |
There was a problem hiding this comment.
I like that you implemented a feature that fetches more recipes if you scroll down!
However, there seems to be a small bug here when I have selected a filter and scrolls down to the bottom of the page, it will fetch new recipes and show all. I see that for the random button you turn off the scroll function when the button is clicked and turn it back on after a delay. Maybe you could do something similar for the filter buttons? Another solution would be to only use the scrollEnable variable and update the value when you want the scroll to be active (scrollEnable=true), eg when site is loaded and when the "all button" is selected. And then inactivate scroll (scrollEnable=false) when filters, sorting and random are active.
| // Event listener for scroll | ||
| window.addEventListener('scroll', () => { | ||
| // checking if the user is near the end of the page | ||
| if (scrollEnabled && window.innerHeight + window.scrollY >= document.body.offsetHeight - 100) { | ||
| // loading more recipes | ||
| if (recipes.length >= currentPage * recipesPerPage) { | ||
| currentPage++; | ||
| fetchRecipes(); | ||
| } | ||
| setTimeout(() => { | ||
| scrollEnabled = true; // Re-enable scroll after a delay | ||
| }, SCROLL_DELAY); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Again, I really like this feature :) Good explanations in the code
| color: white; | ||
| border: 2px solid #4a43460f; | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe you could add an "active" class on the diet filter buttons too (like you have for the cuisine filter and sorting) to make it more clear which filter is selected
| border-radius: 10px; | ||
| box-shadow: 0 4px 6px rgba(0, 0, 0, 0.1); | ||
| max-width: 288px; | ||
| width: 100%; |
There was a problem hiding this comment.
I noticed a small style issue on larger screens: for some widths (ca 928-1125px and ca 1248-1512px), the cards seems to span over the vertical gaps i the grid. I think this can be fixed by removing this width 100% on the cards. I tried to remove it and I think it still looks good on smaller screens :)
| .filters { | ||
| flex-direction: row; | ||
| justify-content: flex-start; | ||
| align-items: center; |
There was a problem hiding this comment.
This line can be removed, you already have align-items:center in the mobile default style (on line 29)
| } | ||
|
|
||
| .card { | ||
| max-width: 288px; |
There was a problem hiding this comment.
Can also be removed, same max-width as in the mobile style
3 participants
Please review my project :)