-
Notifications
You must be signed in to change notification settings - Fork 60
Code review project-recipe-library #45
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
|
Och här är länken till Netlify: https://marina-recipelibrary.netlify.app/ |
JeffieJansson
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.
Väldigt fint jobbat! Jag är ingen JavaScript-expert, men jag tyckte att din kod var väldigt tydlig och lätt att följa och känns som att du har fått till både struktur och funktion på ett grymt sätt. 🙌
Så bra jobbat Marina!
style.css
Outdated
| } | ||
|
|
||
| .card:hover { | ||
| transform: scale (1.03); |
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.
Jag upptäckte ett extra mellanslag mellan scale och (1.03); :)
script.js
Outdated
| .filter(recipe => selectedCuisine === "All" || recipe.cuisines.includes(selectedCuisine)) //Filter on chosen cuisine | ||
|
|
||
| if (currentRecipes.length === 0) { | ||
| container.innerHTML = `<p No recipes was found for this "${selectedCuisine}"😕</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.
Upptäckte en saknad “>” i tomt-meddelande på <p, enkelt att sånt kan hända :)
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!
Your code is well structured and easy to follow.
Remember to stick to one way of creating your functions, either with the function keyword or arrow functions.
Keep up the good work!
| .btn-sorting:hover { | ||
| border: 2px solid #2d46d5; | ||
| } |
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 "hopping" of the buttons when you hover over them you could add the same border on all the buttons but transparent color:
border: 2px solid transparent;
So the border is there all the time but only visible on hover. I hope that makes sense?
| .btn-random:hover { | ||
| border: 2px solid #2d46d5; | ||
| } |
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 you want to refactor the code and keep it more DRY you could stack similar styling together.
since this is exactly the same styling as for btn-sorting:hover
not neccesary just something to take with you
| //======= FUNCTIONS for UI =======// | ||
|
|
||
| //Buttons, changes color when active// | ||
| function makesButtonInteractive(groupClass) { |
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.
Either stick to functions declared like:
function myFunctionName () {}
or
const myFunctionName = () => {}
don't mix
| container.innerHTML = "" //Resetting the container before filling it// | ||
|
|
||
| recipeArray.forEach(recipe => { | ||
| const cuisineText = recipe.cuisines.length ? recipe.cuisines.join(", ") : "N/A" |
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.
smart!
| //Filterbuttons// | ||
| buttons.forEach(button => { | ||
| button.addEventListener("click", () => { | ||
| selectedCuisine = button.textContent.trim() //trim removes invisible spaces at the beginning or end of a text |
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 safety to trim
| //======= FETCH from API =======// | ||
|
|
||
| async function fetchRecipes() { | ||
| container.innerHTML = "<p>⏳ Loading recipes...</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.
loading state! ⭐
Hej Jennifer!
Här är mitt projekt. Jag är inte klar än. Kämpar fortfarande en del med att få sidan responsive 😎 Du kanske har magiska tips!
Kram Marina