Recipe library - Julia.L #55
Conversation
sandrahagevall
left a comment
sandrahagevall
left a comment
There was a problem hiding this comment.
Awesome job with the recipe library! The page looks really great, and I really like the drop-down for the ingredients—such a nice touch. Overall, your code is well-structured and easy to follow since you keep things organized throughout. Really well done—you should be proud of what you’ve done! 🥇
| .text-container { | ||
| display: flex; | ||
| flex-direction: row; | ||
| gap: 88px; | ||
| } |
There was a problem hiding this comment.
If you use grid-template-columns: repeat(auto-fit, minmax(...px, 1fr)); from the beginning it could be easier when the screen grows, instead of changing from grid to flexbox for bigger screens. Or, if prefered, use flexbox from the beginning and change from columns to row, for example :) Could be easier to maintain in that way.
| .container { | ||
| display: flex; | ||
| flex-direction: row; | ||
| flex-wrap: wrap; | ||
| gap: 16px; | ||
| } |
There was a problem hiding this comment.
Since you already have defined display: flex and flex-wrap in the original code, you don't have to write it again in the media query. It's enough to only write the code that will change from the original code.
There was a problem hiding this comment.
But since its mobile first it stays in one row up until desktop if I remove it, I can't say that I know why but that's what happend so I left it there haha
| @media (min-width: 1431px) { | ||
|
|
||
| .container { | ||
| display: flex; | ||
| flex-direction: row; | ||
| flex-wrap: wrap; | ||
| gap: 20px; | ||
| } | ||
|
|
||
| .text-container { | ||
| display: flex; | ||
| flex-direction: row; | ||
| gap: 88px; | ||
| } |
There was a problem hiding this comment.
Same goes for the .container—it’s enough to only specify gap, since that’s the only property that changes. As for .text-container, nothing changes above 1431px, so the code in that media query is redundant.
| <details> | ||
| <summary><b>Ingredients</b></summary> | ||
| <li><br>${recipe.extendedIngredients.map(ingredients => ingredients.name).join("<br>")}</li> | ||
| </div>` |
There was a problem hiding this comment.
Really nice touch with this drop-down for the ingredients! 💯
| <h1>Recipe Library | ||
| </h1> | ||
|
|
||
| <div class="text-container"> |
There was a problem hiding this comment.
When I was going through your code, I had to go back a few times to remember what .text-container was. Just a tip 👍: it might be easier to define it in a way that more clearly indicates what it contains, so that someone new to your code can understand it more easily. When it’s just called 'text'-container, it might not be so self-explanatory.
style.css
Outdated
| .example { | ||
| width: 100%; | ||
| border-radius: 12px; | ||
| } |
There was a problem hiding this comment.
Do you need this? Can't find any "example" in you HTML, I guess it might be left from the beginning of this project?
There was a problem hiding this comment.
No you are right, that's left over from when I used the example before I got my backup to work! Thanks for noticing!
| console.warn('API returned empty, using backup data.') | ||
| allRecipes = backUpData | ||
| } | ||
| } catch (e) { | ||
| console.error('Fetch failed, using backup data.', e) | ||
| allRecipes = backUpData |
There was a problem hiding this comment.
It might be good to show these types of error messages for the user as well, not only in the console :)
| @@ -0,0 +1,129 @@ | |||
| // Import backUpData | |||
| import { backUpData } from "./backUpData.js" | |||
There was a problem hiding this comment.
Smart that you added back up data, so that you have something to fall back on 💯
style.css
Outdated
| .filter, | ||
| .sort, | ||
| .random { | ||
| grid-row: span 1; | ||
| } |
style.css
Outdated
| font-style: medium; | ||
| font-weight: 500; |
There was a problem hiding this comment.
You can remove font-style: medium, it doesn't do anything in CSS and you aldready have font-weight.
| .text-container { | ||
| display: grid; | ||
| grid-template-columns: repeat(1, 350px); | ||
| grid-template-rows: auto; | ||
| } |
There was a problem hiding this comment.
It could be a good idea to use, for example, grid-template-columns: repeat(auto-fit, minmax(...px, 1fr)); instead, for better responsiveness to the website.
Also, grid-template-rows: auto; is not needed here because, by default, CSS Grid sets rows to auto.
JennieDalgren
left a comment
JennieDalgren
left a comment
There was a problem hiding this comment.
Great job with this project!
Your code is clean and easy to follow.
You have managed to reach all requirements and some stretch goals too!
Keep up the good work 💪
| console.log("backUpData loaded", backUpData) | ||
|
|
||
|
|
||
| // Pickups |
| if (!Array.isArray(recipes) || recipes.length === 0) { | ||
| container.innerHTML = `<p>The filter you chose doesn't match a recipe.</p>` |
There was a problem hiding this comment.
good to code defensively and prepare of empty results like this
| <div class="card"> | ||
| <img src="${recipe.image}"/> | ||
| <h2>${recipe.title}</h2> | ||
| <hr class="solid"> | ||
| <li><b>Cuisine: </b>${recipe.cuisines}</li> | ||
| <li><b>Time: </b>${recipe.readyInMinutes}</li> | ||
| <hr class="solid"> | ||
| <details> | ||
| <summary><b>Ingredients</b></summary> | ||
| <li><br>${recipe.extendedIngredients.map(ingredients => ingredients.name).join("<br>")}</li> | ||
| </div>` |
There was a problem hiding this comment.
you can still work with some indentations in the html even here in the js file
|
|
||
| // Filter & Sort | ||
| const updateUI = () => { | ||
| let visible = [...allRecipes] |
3 participants
Netlify link: https://recipeslibrary.netlify.app/