-
Notifications
You must be signed in to change notification settings - Fork 32
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
JS-Recipe-Library-Project #33
base: main
Are you sure you want to change the base?
Conversation
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 know you're far from done so I know you've probably thought about some of the things I mentioned already. I feel like you on a good path when it comes to the javascript. Its mostly the styling that need a bit of fixing up. :)
I think it would benefit you to try to style it mobile-first and then do media queries for bigger screens. Maybe even put the sort-buttons under the filter buttons or just flex-wrap the buttons so we can see all of them at once. A minor detail might also be that the recipes pictures on some widths, become huge. Love the shadows and transformation on hovering on the buttons though. Great job and keep at it!
Wishing you a great weekend!
<label class="filter-label"> | ||
<input type="radio" name="cuisine" value="all" checked class="filter-input"> | ||
<span class="filter-button" data-type="">All</span> | ||
</label> |
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.
All of the html needs a bit of cleanup. Looks like some of the indentations are 2 spaces and some are 4 spaces. It would be good to stick with 2. The element that have several attributes should have the attributes on different lines. Eg.
… and so on.
<div class="filter-group"> | ||
<div class="filter-title">Sorting options</div> | ||
<div class="filter-options"> | ||
<label class="sort-label"> | ||
<input type="radio" name="sort" value="time" checked class="sort-input"> | ||
<span class="sort-button">Cooking time</span> | ||
</label> | ||
<label class="sort-label"> | ||
<input type="radio" name="sort" value="popularity" class="sort-input"> | ||
<span class="sort-button">Popularity</span> | ||
</label> | ||
<label class="sort-label"> | ||
<input type="radio" name="sort" value="price" class="sort-input"> | ||
<span class="sort-button">Price</span> | ||
</label> | ||
<button id="random">Random Recipe</button> | ||
</div> | ||
</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.
you have chosen really good class names that makes sense! well done
<div class="card" data-type="italian"> | ||
<img src="images/avocado.jpg" alt="Food Image"> | ||
<div class="card-title">Cheat's cheesy Focaccia</div> | ||
<div class="divider"></div> | ||
<div class="details"> | ||
<div class="detail-item"> | ||
<div class="detail-label">Cuisine:</div> | ||
<div class="detail-value">Italian</div> | ||
</div> | ||
<div class="detail-item"> | ||
<div class="detail-label">Time:</div> | ||
<div class="detail-value">40 minutes</div> | ||
</div> | ||
</div> | ||
<div class="divider"></div> | ||
<div class="ingredients-title">Ingredients</div> | ||
<div class="ingredients-list"> | ||
500 pack bread mix<br> | ||
2 tbsp. olive oil, plus a little extra for drizzling<br> | ||
25g parmesan (or vegetarian alternative), grated<br> | ||
75g dolcelatte cheese (or vegetarian alternative) | ||
</div> | ||
</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.
this part is not ncessary any more, but you probably know that already ;)
const response = await fetch(apiURL,); | ||
|
||
if (!response.ok) { | ||
throw new Error("Failed to fetch recipes. "); | ||
} | ||
|
||
const data = await response.json(); | ||
|
||
if (!data.recipes) { | ||
recipesContainer.innerHTML = "<p>Daily API limit reached. Try again later.</p>"; | ||
return; | ||
} | ||
|
||
recipes = data.recipes; // Store fetched recipes globally | ||
renderRecipes(recipes); | ||
} catch (error) { | ||
console.error("Error fetching recipes:", error); | ||
recipesContainer.innerHTML = "<p>Failed to load recipes. Try again later.</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.
nice error-handeling in this one!
Maybe add some more in case a recipie doesn't have a value somewhere. There was for example one recipie where the picture didnt show.
<h2>${recipe.title}</h2> | ||
<p>Ready in ${recipe.readyInMinutes} minutes</p> | ||
<p>Servings: ${recipe.servings}</p> | ||
<a href="${recipe.sourceUrl}" target="_blank">View Recipe</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.
I really like that the recipe opens up in a new tab! Its a nice touch and something me as a user would appreciate
@media (max-width: 768px) { | ||
main { | ||
padding: 20px; | ||
} | ||
|
||
.recipe-card { | ||
width: 100%; | ||
} | ||
} |
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.
Maybe the mobile styling in the top and then do min-width down here instead for bigger screens.
.card img { | ||
width: 100%; | ||
height: 200px; | ||
background: linear-gradient(0deg, #899CCC 0%, #899CCC 100%); |
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.
what does this do on the image? I'm just curious :)
|
||
.filter-button:hover { | ||
box-shadow: 0px 0px 30px rgba(0, 23.74, 163.62, 0.4); | ||
transform: translateY(-3px); |
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.
is this the part that makes the buttons look like they're lifting from the page? Its a cool feature
</div> | ||
|
||
<div class="filter-group"> | ||
<div class="filter-title">Sorting options</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.
maybe put all text in a p-tag or an h-tag? You can still use the div as well though if you need to position them in a certain way.
.filter-container { | ||
width: 694px; | ||
height: 85px; | ||
display: inline-flex; |
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.
Im not sure if this is correct, but I think when you use inline-flex, you force the elements to be in one line which in this case does so you cant see the buttons at the end without scrolling to the right. This happens both in mobile and desktop view.
#https://resonant-boba-03de0c.netlify.app/