Recipe library - pull request #31
Conversation
… the ingredients-array
…dd function so that all-button is unchecked when others are
…n the clicked buttons id.
…ies to be able to filter on several filters at the same time.
…-object rather than string of id's or diets, like it was before
…ce rather than comma and no space
…cipe function to match
…he important bits
index.html
Outdated
There was a problem hiding this comment.
I love how the recipes are displayed, being kinda "miss-matched" instead of in uniform rows
style.css
Outdated
| -Use locar storage instead of fetching every time, CHECK | ||
| -Add filter for Cuisine | ||
| -Add the source and pointer for hovering on recipes CHECK | ||
| -Show Loading satate while loading data. CHECK |
There was a problem hiding this comment.
Just had a question about this since it's checked. I was not able to view this state, altough that might simply be because you use local storage. Idk, just a thought!
There was a problem hiding this comment.
I did comment out the local storage to try it and then it worked so I hope it still does xD
script.js
Outdated
| }) | ||
| } | ||
|
|
||
| //Här börjar allt från att sidan laddas |
There was a problem hiding this comment.
I don't know if comments count, but try and keep it in english
There was a problem hiding this comment.
ooh really good point! I missed that!
| const randomRecipe = () => { | ||
| const randomIndex = Math.floor(Math.random() * workingArray.length) | ||
| workingArray = [workingArray[randomIndex]] | ||
| loadRecipes(workingArray) | ||
| workingArray = [...allRecipes] | ||
| } |
There was a problem hiding this comment.
after loading a random recipe, workingArray is set back to all recipes. While it only shows one recipe, internally all of them are loaded. Could be re-written like this i think:
const randomRecipe = () => {
const randomIndex = Math.floor(Math.random() * workingArray.length)
const selectedRecipe = workingArray[randomIndex]
loadRecipes([selectedRecipe])
};
There was a problem hiding this comment.
You're absolutely right! The reason why I did it like that is because I wanted the user to get a completly random recipe from "all" recipes rather than a random recipe of for example vegan recipes. But now when you mentioned it might make more sense to get a random recipe WITH a filter if the user wants that. Thanks you <3
| workingArray = activeFilters.length > 0 | ||
| ? workingArray.filter(recipe => activeFilters.every(filter => recipe[filter] === true)) | ||
| : [...allRecipes]; |
There was a problem hiding this comment.
ChatGPT flags these lines and seems to think allRecipes should be filtered instead of workingArray, as it can cause issues if multiple filters are selected in succesion. Altough, i don't understand his logic, so i'll leave it as a thought
There was a problem hiding this comment.
haha ok interesting! Will have a look at that thanks
There was a problem hiding this comment.
Nice and descriptive function and variable names 👍 Except for maybe anyChecked and anyCheckedNew. They could probable be named something more explanatory?
I can’t see you’ve met this requirement: “Your page should have an empty state, e.g. if there are no recipes matching the filter - show a meaningful message to the user”.
I can see you’ve followed the design, but if you have some more time I would love to see some more space in your styling. Have another look at the design and add some “air”. I think that thing alone will make your page look a lot nicer 😇
The instructions expansion doesn’t work quite as expected - it looks a little wonky and make things jump around a bit. I’ll share a video to showcase.
Changes requested
- Missing requirement: empty state
Still have some work to do and haven't focused too much on the styling. Would love any feedback I can get on this area anyways, of course. Please ignore the gray hearts. My plan is to save some recipies in a/(another) local storage so that they can save their favourite recipie. But yeah, It's far from done yet x )
Let me know if you have any questions!