Skip to content
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 project recipe library (Kasia) #13

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

Kasssiiii
Copy link

JS project recipe library - pull request after W3

https://js-recipe-libr.netlify.app/

Copy link

@JasminHed JasminHed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello dear Kassia,
Wonderful work with testing and trying things out and building step by step. I can see through your code that that is how you have worked. Great work in general!! I have a few things (nothing request change from my side).

  • Clean and understandable HTML, just had 1 question
  • Clean CSS, just had 1 comment on mediaqueires. Should there be more mediaquieres?
  • Whenever you (we) feel ready, lets not forget to remove the commented out sections
  • I really appreciate your structure in JS code. You seem to have them in sections and that really shows, eg; button and so on. I also found some inspiration on how you displayed recipes. Thank you for that! One thing, does the fetch and .then . then logic exist in the JS document?

Like I wrote, excellent work and the page looks really good! One step at the time is definitly the way to go.

Comment on lines +42 to +44

<section class="errorMessage" id="errorMessage"></section>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, why is this here? I am not suggesting it is wrong, just that I did it another way...I think and so find it useful to understand how we can find the same solution but in a different way. Thank you!

Comment on lines +100 to +110
.recipeSection {
/*display: flex;*/
display: grid;
grid-template-columns: repeat(auto-fill, minmax(278px, 1fr));
/*width: 300px;*/
padding: 16px 16px 24px 16px;
/*flex-direction: row;
align-items: flex-start;*/
gap: 28px;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the comment out for testing. I use it too! Just a kind reminder that whenever we are done with testing and trying things out we remove the comments out. I forget all the time ❤️

Comment on lines +40 to +44
@media (min-width: 530px) {
.filters {
display: flex;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this (mediaqueries) be at the end of the css document, after everything else, perhaps? Hum... I might be missing something here. Please correct me if I am misunderstanding :)

Comment on lines +81 to +97
let recipes = [];
//fill in the recipes array with the data from the API (only picked fields needed for button selection)
//all the "Filter on kitchen" butttons are contained within types (dish types)
apiRecipes().then((data) => {
recipes = data.map((recipe) => {
return {
title: recipe.title,
types: recipe.dishTypes.filter( element => allFilters.includes(element)),
image: recipe.image,
readyInMinutes: recipe.readyInMinutes,
servings: recipe.servings,
diets: recipe.diets,
cuisine: recipe.cuisines,
ingredients: recipe.extendedIngredients.map((ingredient) => ingredient.original),
pricePerServing: recipe.pricePerServing,
popularity: recipe.aggregateLikes,
url: recipe.spoonacularSourceUrl,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am taking note on this, do not have it in the same way and wonder why I did not do it :) Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants