-
Notifications
You must be signed in to change notification settings - Fork 60
Carolinas-recipe-library-pull request #53
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
base: main
Are you sure you want to change the base?
Conversation
…but it was a bit hard
daniellauding
left a comment
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.
Good job! Like the page and code is clear
| <button | ||
| class="meal-btn" | ||
| id="btnAll" | ||
| >All</button> | ||
| <button | ||
| class="meal-btn" | ||
| id="btnBreakfast" | ||
| >Breakfast</button> | ||
| <button | ||
| class="meal-btn" | ||
| id="btnLunch" | ||
| >Lunch</button> | ||
| <button | ||
| class="meal-btn" | ||
| id="btnFika" | ||
| >Fika</button> | ||
| <button | ||
| class="meal-btn" | ||
| id="btnDinner" | ||
| >Dinner</button> |
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 understand why the buttons are broken over multiple lines, but since there’s no nested content or long attributes, it might be cleaner to keep each on a single line for readability.
Like
Button text instead :D| const btnAll = document.getElementById("btnAll") | ||
| const btnBreakfast = document.getElementById("btnBreakfast") | ||
| const btnLunch = document.getElementById("btnLunch") | ||
| const btnFika = document.getElementById("btnFika") | ||
| const btnDinner = document.getElementById("btnDinner") | ||
| const btnAscending = document.getElementById("btnAscending") | ||
| const btnDescending = document.getElementById("btnDescending") | ||
| const mealBtns = document.querySelectorAll(".meal-btn") | ||
| const timeBtns = document.querySelectorAll(".time-btn") | ||
| const randomBtn = document.getElementById("btnRandom") | ||
| const recipeContainer = document.getElementById("recipeContainer") | ||
| const noResultsContainer = document.getElementById("noResultsContainer") | ||
| const loading = document.getElementById("loading") | ||
| const searchInput = document.getElementById("searchBar") | ||
| const searchBtn = document.getElementById("searchBtn") |
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.
Clean structure to find the DOM selectors!
| <div class="recipe-card"> | ||
| <img src="${recipe.image}" alt="${recipe.title}"> | ||
| <h3>${recipe.title}</h3> | ||
| <hr> |
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.
Not sure I would do an
here — it introduces an extra DOM element and forces a visual line even if future designs might not include one. Since the goal is just a horizontal divider, it might be more flexible and semantic to use a CSS border or pseudo-element instead. That way, you can easily adjust or remove the line in future design updates without changing the HTML structure.
| const cacheTime = localStorage.getItem("cacheTime") | ||
|
|
||
| const now = Date.now() | ||
| const sixHours = 6 * 60 * 60 * 1000 // cache duration |
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 with setting a duration for cache
| btnAll.addEventListener("click", () => { activeFilters.mealType = "All"; filterAndSorting() }) | ||
| btnBreakfast.addEventListener("click", () => { activeFilters.mealType = "breakfast"; filterAndSorting() }) | ||
| btnLunch.addEventListener("click", () => { activeFilters.mealType = "lunch"; filterAndSorting() }) | ||
| btnFika.addEventListener("click", () => { activeFilters.mealType = "fika"; filterAndSorting() }) | ||
| btnDinner.addEventListener("click", () => { activeFilters.mealType = "dinner"; filterAndSorting() }) | ||
| btnAscending.addEventListener("click", () => { activeFilters.sortOrder = "asc"; filterAndSorting() }) | ||
| btnDescending.addEventListener("click", () => { activeFilters.sortOrder = "desc"; filterAndSorting() }) | ||
| randomBtn.addEventListener("click", getRandomRecipe) | ||
| searchBtn.addEventListener("click", searchRecipes) | ||
| searchInput.addEventListener("keypress", (e) => { if (e.key === "Enter") searchRecipes() }) |
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.
Good grouping of event listeners 👍 — one thought is that as this logic scales, it could be more maintainable to encapsulate these within a component or a setup function related to the feature (e.g. initMealFilters() or similar). That way, each feature owns its own event bindings.
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.
Adding on to the comment here.
If you want to refactor this could be an option:
Create a helper function that updates the activeFilters object with the given properties, then run the filterAndSorting function
const applyFilters = updates => {
Object.assign(activeFilters, updates)
filterAndSorting()
}
You can create something called a Map that pairs each button element with its meal type (read more here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map)
const mealButtons = new Map([
[btnAll, 'All'],
[btnBreakfast, 'breakfast'],
[btnLunch, 'lunch'],
[btnFika, 'fika'],
[btnDinner, 'dinner']
])
Then loop through the Map
for each [button, mealType] pair, add a click listener
when clicked, it updates activeFilters.mealType and calls filterAndSorting()
mealButtons.forEach((mealType, btn) => {
btn.addEventListener('click', () => applyFilters({ mealType }))
})
Have a read thru if you want to, for future reference.
JennieDalgren
left a comment
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.
Really good job with this project. It seems like you had a lot of fun! Nice small details like loading state, spread operator, local storage and cache limit
Keep up the good work!
| btnAll.addEventListener("click", () => { activeFilters.mealType = "All"; filterAndSorting() }) | ||
| btnBreakfast.addEventListener("click", () => { activeFilters.mealType = "breakfast"; filterAndSorting() }) | ||
| btnLunch.addEventListener("click", () => { activeFilters.mealType = "lunch"; filterAndSorting() }) | ||
| btnFika.addEventListener("click", () => { activeFilters.mealType = "fika"; filterAndSorting() }) | ||
| btnDinner.addEventListener("click", () => { activeFilters.mealType = "dinner"; filterAndSorting() }) | ||
| btnAscending.addEventListener("click", () => { activeFilters.sortOrder = "asc"; filterAndSorting() }) | ||
| btnDescending.addEventListener("click", () => { activeFilters.sortOrder = "desc"; filterAndSorting() }) | ||
| randomBtn.addEventListener("click", getRandomRecipe) | ||
| searchBtn.addEventListener("click", searchRecipes) | ||
| searchInput.addEventListener("keypress", (e) => { if (e.key === "Enter") searchRecipes() }) |
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.
Adding on to the comment here.
If you want to refactor this could be an option:
Create a helper function that updates the activeFilters object with the given properties, then run the filterAndSorting function
const applyFilters = updates => {
Object.assign(activeFilters, updates)
filterAndSorting()
}
You can create something called a Map that pairs each button element with its meal type (read more here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map)
const mealButtons = new Map([
[btnAll, 'All'],
[btnBreakfast, 'breakfast'],
[btnLunch, 'lunch'],
[btnFika, 'fika'],
[btnDinner, 'dinner']
])
Then loop through the Map
for each [button, mealType] pair, add a click listener
when clicked, it updates activeFilters.mealType and calls filterAndSorting()
mealButtons.forEach((mealType, btn) => {
btn.addEventListener('click', () => applyFilters({ mealType }))
})
Have a read thru if you want to, for future reference.
| const fetchData = async (URL) => { | ||
| loading.style.display = "block" | ||
| recipeContainer.innerHTML = `` | ||
| noResultsContainer.innerHTML = `` | ||
|
|
||
| try { | ||
| const cachedData = localStorage.getItem("cachedRecipes") | ||
| const cacheTime = localStorage.getItem("cacheTime") | ||
|
|
||
| const now = Date.now() | ||
| const sixHours = 6 * 60 * 60 * 1000 // cache duration |
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.
⭐
|
|
||
| // Filter by meal type | ||
| if (activeFilters.mealType && activeFilters.mealType !== "All") { | ||
| filtered = filtered.filter(recipe => recipe.dishTypes?.includes(activeFilters.mealType)) |
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.
⭐
| noResultsContainer.innerHTML = ` | ||
| <div class="no-results-container"> | ||
| <h3>No recipes found for "${activeFilters.mealType}"</h3> | ||
| <p>Try another filter</p> | ||
| </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.
good UX
| return | ||
| } | ||
|
|
||
| let filtered = [...recipeInfo] |
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.
⭐ using spread operator!
| // Use cache if still valid | ||
| if (cachedData && cacheTime && now - cacheTime < sixHours) { | ||
| recipeInfo = JSON.parse(cachedData) | ||
| loading.style.display = "none" |
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.
like the way you're handling the loading state
netlify link: https://carolinas-recipe-library.netlify.app/