-
Notifications
You must be signed in to change notification settings - Fork 60
js-project-recipe-library-frida #50
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
|
Overall Review: |
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.
Good job with the project!
It is tricky to actually filter on some preferences when using this random endpoint so i had to refresh a couple of times. But that's the API 🫣
Think about being consistent when you declare you functions. Either use arrow functions or function keyword. Do not mix
Keep up the good work!
| <div class="header"> | ||
| <h1>Recipe Library</h1> | ||
| </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.
use the
instead of div if it is a header| group.querySelectorAll("button").forEach(b => b.classList.remove("selected")) | ||
| e.target.classList.add("selected") |
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 but maybe it could work to use .toggle("className") here
|
|
||
| // ------ Data from localStorage as backup if API fails ------ // | ||
| function loadFromLocalStorage() { | ||
| const storedRecipes = localStorage.getItem("recipes") |
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.
⭐
|
|
||
|
|
||
| // ------ Recipe cards ------ // | ||
| function renderMeals(meals) { |
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.
stick to either arrow functions like youve done in
const fetchData = () => {...}
or
function renderMeals(meals) {...}
dont mix
| if (meals.length === 0) { //no matching recipes - message | ||
| recipeCard.innerHTML = '<p class="error">No recipe match, sorry 🫣</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.
good fallback!
| border: none; | ||
| cursor: pointer; | ||
| border-radius: 50px; | ||
| padding: 8px 16px 8px 16px; |
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.
padding: 8px 16px 8px 16px;
can be shortened to
padding: 8px 16px;
vertical and horisontal values when they are two
Here's my project and netlify.
(obs, I see that netlify is not yet synced with the latest version as per thursday 17.50 🫣)