-
Notifications
You must be signed in to change notification settings - Fork 60
Recipe library - Asako #60
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
index.html
Outdated
| placeholder="Find a recipe or ingredient"> | ||
| <button | ||
| type="button" | ||
| class="search-button"><span class="search-emoji">🔍</span></button> |
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.
Suggesting a new line for span and another one for the closing tag for the button here :)
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.
Fixed!:D
index.html
Outdated
| <div id="recipe-container"> | ||
| Loading... | ||
| </div> | ||
| <p id="no-results" style="display: none;">Oops... 😣 No recipes found</p> |
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.
style="display: none; <---- Would maybe add a class here instead, that you can style in CSS and control in JS, instead of putting some CSS inside the HTML :)
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 added "display: none" in CSS instead :) And It actually needs to be id here because I want to use this message only for the search function. I have several error messages with class="no-results", and if this no-results had class, it would be shown with other error message which has the same class when something went wrong. To avoid that I use id here :)
gabriellaberko
left a comment
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.
To summarize - I am amazed, you have done a great job with this project! The code is structured, clean and easy to follow, and you have mastered all the JavaScript functionality and achieved a responsive design. From what I can see it seems to meet all the assessment criteria + stretch goals.
Again, well done! Was a pleasure to review it!
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.
Overall great structure, super clean and readable! The naming of elements is great and self-explanatory :) Great work!
| } | ||
|
|
||
| /* ====================================== | ||
| favorite recipes button |
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.
Love these comments for a great overview of where the styling is applied!
| favorite recipes button | ||
| ======================================= */ | ||
|
|
||
| .favorites { |
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.
A small nice-to-have is to add a similar hoving effect & active state to the favorite button, as the other buttons have! :)
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.
Added a border and box shadow :) Thank you for the tips!
| style for the buttons | ||
| ======================================= */ | ||
|
|
||
| .button-container { |
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 would consider to add a gap property to the button-container to have some more spacing between the buttons - but probably in a media query. The buttons look great, but look a little bit squeezed on a wider screen (more specifically wider than 1000px) :)
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 didn't notice that and now I added a gap :) Thank you! It looks better this way!
| margin: 20px 0; | ||
| } | ||
|
|
||
| .view-recipe-button { |
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.
Love the idea of having a button that links out to the full recipe!
| // save favorite recipes in localStorage | ||
| let favorites = JSON.parse(localStorage.getItem("favorites")) || []; | ||
|
|
||
| const attachLikeEvents =() => { |
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.
Great work with the favorite functionality! And the comments makes it really easy to follow. Impressed that you got it to work well with the local storage for it as well! The only comment I have here, is that it would be nice to keep the active state of the hearts when you click to see your favorite recipes. It is a little hard to see if you remove them from there, even though the functionality is there (to add and remove) :) Besides that small thing, it works great! Well done!
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 are right, it would look better in your way! Thank you for trying and checking all functions:) I would definitely try that if I had more time:D
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 was thinking exactly this. The UX of that favorites view confused me a bit since the red color didnt follow to this view.
| if (favorites.length > 0) { | ||
| displayRecipes(favorites); | ||
| } else { | ||
| container.innerHTML = `<p class="no-results"> No favorite recipes yet ❤️</p>`; |
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.
Great idea to have a specific message here, instead of the default error message of no recipes matching the filter!
| //=============================== | ||
|
|
||
| // change the color of the button when clicked | ||
| randomBtn.addEventListener("click", () => { |
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.
Would be nice to try to remove the active state from the random button when you select something else (another filter button etc.). And also for it to stay active when you press it several times. Now it seems to toggle between active/inactive state even if you keep randomizing more recipes with the button, and it also stays active when you choose another filter option :)
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.
Thank you for checking this! I didn't notice it by myself.
| let matchFound = false; | ||
|
|
||
| searchTargets.forEach((target) => { | ||
| const text = target.textContent.toLowerCase(); |
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.
Search function works great! Love that you are able to search in the whole text content!
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.
Really great work with the JavaScript! You seem to have mastered it already. Very clean, readable and structured like everything else!
I have a few comments below, regarding the active state of the random button and the like buttons - but the actual functionality of all the sorting and filtering (including the favorites) works great regardless!
|
Thank you so much for your positive and constructive feedback!:D |
JennieDalgren
left a comment
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.
Great job with this project! Not only have you nailed all the requirements and several stretch goals, your code is very well structured and easy to follow.
I've added a few comments, mainly some small tips on how you can refactor your css a bit. And You got useful tips from your peer reviewer too.
Keep up the great work! 🚀
| background-color: #ccffe2; | ||
| color: #0018a4; | ||
| font-size: 18px; | ||
| border: none; |
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.
To avoid the "jumping" of the buttons when you hover, since you add a border of 2px on hover, you could change border: none; here to instead say border: 2px solid transparent; that way it has an invisible border all the time, but you don't have the jumping when a button is hovered. This goes for both wort and filter btns ofc.
| .random-btn { | ||
| text-align: center; | ||
| background-color: beige; | ||
| color: #0018a4; | ||
| font-size: 18px; | ||
| border: none; | ||
| width: auto; | ||
| height: 40px; | ||
| border-radius: 50px; | ||
| padding: 8px 16px; | ||
| margin: 5px; | ||
| cursor: pointer; |
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 looks like the different buttons share most ofd the styling. You could have a general button class that takes care of the base styling and then just style the random-btn, sort-btn etc with the styles that is unique that that specific button. To avoid repeating yourself. (Keep it DRY)
| // save favorite recipes in localStorage | ||
| let favorites = JSON.parse(localStorage.getItem("favorites")) || []; | ||
|
|
||
| const attachLikeEvents =() => { |
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 was thinking exactly this. The UX of that favorites view confused me a bit since the red color didnt follow to this view.
| // check if the recipe is already in favorites | ||
| if (favorites.some(fav => String(fav.id) === recipeId)) { | ||
| // remove recipe if it is already liked | ||
| favorites = favorites.filter(fav => String(fav.id) !== recipeId); | ||
| button.classList.remove("liked"); | ||
| } else { | ||
| // otherwise add recipe to my favorites | ||
| favorites.push(recipe); | ||
| button.classList.add("liked"); | ||
| } |
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.
⭐
| } else { | ||
| container.innerHTML = `<p class="no-results">No recipes found 🥲 Check your API key or quota.</p>`; | ||
| } |
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.
⭐
| descBtn.addEventListener("click", () => { | ||
| currentSort = "desc"; | ||
| applyKitchenFilters(); | ||
| }); | ||
|
|
||
| ascBtn.addEventListener("click", () => { | ||
| currentSort = "asc"; | ||
| applyKitchenFilters(); | ||
| }) |
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.
Super! Great that you are storing the current sort like that. That males it persist even when you change the cuisine filter.
Link to Netlify : https://js-project-recipe-library-site.netlify.app/