Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

js-project-recipe-library by Oscar #21

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

Conversation

osckli990
Copy link

#https://js-project-recipe-library.netlify.app

My recipe library:

  • basic design following the figma template
  • filtering and sorting options with the ability to combine them
  • pagination!

Still got issues to fix, which will be updated this or the coming week

…avascript, styled some more, finished HTML. will add media query later
… and cost. redid cost to be fewer buttons, when selecting diets, multiple can be selected as well as reloading the original recipes if unchecked
…ecipe for a button i created earlier. created a very specific if statement for a combination of sorting that would result in no recipes being shown. will change when i have time
… checked and unchecked. was a problem with sorting by time, as the value of the recipe was higher than the max sorting value of 60. added comment to HMTL
…work, it currently kinda works with loading new recipes but contains duplicates of stored recipes. Reworked the update function to contain the selected- diet/cost/time to make the code shorter. currently iäm also re-declaring let storedRecipes a multitude of times. could be improved, probably. added links to innerHTML. will need to comment and re-comment all code to better understand it and make it easier for others
… decimals, need to comment code, shall try writing code more concise and learn how that works. This is mostly a message to myself
…3. leave larger values be. complexsearch seems to return recipes with a different structure to randomsearch
…e-worked updateRecipes and fetchData to be MUCH more concise utilizing .map, .filter, ternary operator and the spread operator (...). these are listed in the order from being able to understand to not understand
Copy link

@TiagoAbrahamsson TiagoAbrahamsson left a comment

Choose a reason for hiding this comment

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

Your code is well structured and logically sound.

The website looks good and resembles the figma design well.

script.js Outdated
@@ -219,7 +218,7 @@ function checkScroll() {
const isFiltered =
checkMix.some(checkbox => checkbox.checked) || // Diet filters
costMix.some(checkbox => checkbox.checked) || // Cost sorting
(selectedTime && selectedTime === "60") // Only block if time is selected & NOT "60-min"
(selectedTime && selectedTime === "60") // Only block if time is selected & not "60-min"

Choose a reason for hiding this comment

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

You have selectedTime written here twice which doesn't seem to be neccessary.

Copy link
Author

Choose a reason for hiding this comment

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

Strange, in my code there's only one line. hmm, i'll commit again i guess

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Good job with this project Oscar!
You've chosen descriptive function names and I can see you spent time on following the design - kudos 👍

Some things to work more on:
Please be consistent with how you define your functions → stick to arrow functions as much as you can. Also, please remove console.log from production code.

Looking through your code, I can’t see you’ve met this requirement 👀:

  • Your page should show a useful message to the user in case the daily API quota has been reached

Also, I found a little bug. I’d expect when I check vegetarian and gluten-free that I’d get recipes that are just that - vegetarian and gluten-free. Buuut, I get a recipe including chicken. Please fix this or make sure that only one diet filter is chosen at a time. For the stretch goal regarding this, it’s enough for me if the filter goes well together with the sortings. Speaking of stretch goals, I can “only” see that you’ve done 1-3 which is totally fine but I just wanted to make sure I’m not missing something if you’re aiming for VG. Let me know!

@osckli990
Copy link
Author

Thank you!

Right, arrow functions. I don't know why i didn't use it consistently.

  • I did have a rudimentary limit, based on calls, changed it to when the API throws the limit error. It should have displayed a message, but now it definetly does

  • Fixed bug with multiple selected diets. It was set to AND and is now OR. So "vegetarian" won't show meat recipes

  • The infinite-scrolling, the 4th requirement, has been fixed and improved. The recipes still loaded before, but it took a long while. improved "loading" icon and changed when it should fetch. a thought being if a complexSearch takes longer time to fetch. Had an error that i've failed to reproduce, where all recipes where fetched at once on scroll-down. Should be fixed

@osckli990 osckli990 requested a review from HIPPIEKICK March 29, 2025 18:51
@HIPPIEKICK
Copy link
Contributor

It was set to AND and is now OR. So "vegetarian" won't show meat recipes

You should probably fix the styling as well now then so that it's clear that the user picks one diet (if you changed the logic to or instead of and 😇).

Apart from that, it looks super!

@osckli990
Copy link
Author

i think i meant it was OR, so if vegetarian and gluten-free was selected it would still show meat, since one of the diets are correct. it is now AND, so if the same diets are selected, it only shows recipes containing both diets

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

Successfully merging this pull request may close these issues.

3 participants