@mickyginger
Posted
Hi Kashish! 👋
You've done a great job here, well done!
Whenever you find yourself repeating chunks of code, it's a sign that you need a function, for example:
allNotesTab.addEventListener("click", () => {
allNotesTab.classList.add("active");
activeNotesTab.classList.remove("active");
completedNotesTab.classList.remove("active");
allListSection.style.display = "block";
activeListSection.style.display = "none";
completedListSection.style.display = "none";
updatingNotes()
});
activeNotesTab.addEventListener("click", () => {
allNotesTab.classList.remove("active");
activeNotesTab.classList.add("active");
completedNotesTab.classList.remove("active");
allListSection.style.display = "none";
activeListSection.style.display = "block";
completedListSection.style.display = "none";
updatingNotes()
});
completedNotesTab.addEventListener("click", () => {
allNotesTab.classList.remove("active");
activeNotesTab.classList.remove("active");
completedNotesTab.classList.add("active");
allListSection.style.display = "none";
activeListSection.style.display = "none";
completedListSection.style.display = "block";
updatingNotes()
});
All the event listeners are the same but you have created three anonymous functions here. Each of these takes up space in memory, and if you want to make a change you have to do it in several places which means that it's quite easy to introduce bugs.
This can be simplified like so:
const handleTabClick = () => {
allNotesTab.classList.add("active");
activeNotesTab.classList.remove("active");
completedNotesTab.classList.remove("active");
allListSection.style.display = "block";
activeListSection.style.display = "none";
completedListSection.style.display = "none";
updatingNotes()
});
allNotesTab.addEventListener('click', handleTabClick);
activeNotesTab.addEventListener('click', handleTabClick);
completedNotesTab.addEventListener('click', handleTabClick);
This creates one function in memory, and changing that one function updates the behaviour of all the tabs at the same time.
I notice you're loading in underscore, but you only seem to be using it in one place in your code. Adding external resources adds weight to your site and while it's not that important for this project, it's good to keep that in mind. The more stuff you load in the slower the site which is often undesirable.
If you just need to check whether an object is empty there are a few ways you can do it. One fairly simple way is:
function isEmpty(obj) {
return JSON.stringify(obj) === '{}';
}
Here's some info on JSON.stringify and what it's doing here.
Finally I notice that you are storing "true" and "false" in localStorage to persist the theme setting. The problem with "true" and "false" is that they are both truthy!
It might be better to store "1" instead of true, and "0" instead of false. You can easily convert these to numbers by using the unary operator. This will allow you to simplify your code:
const isDarkMode = +localStorage.getItem("darkMode");
localStorage.setItem('darkMode', isDarkMode ? 0 : 1);
If you're not sure what isDarkMode ? 0 : 1 is doing checkout the ternary operator
Alright, I appreciate there's a lot of stuff there, hopefully you can make sense of my ramblings 😜
Be sure to let me know if you have any questions! 👍
Marked as helpful
@kashish-d
Posted
@mickyginger Mike, I really really appreciate this! Truthy and Falsy are some keywords, I still need clarification on. But it makes sense to use 0 and 1. And the empty object function, thank you for providing that. I had to use underscore at the last minute just for checking empty objects.
I do have a question, you may have noticed why I had to keep that in repetition, each anonymous function there is actually doing different work. It's adding active class and removing that from the other two but its doing that to different tabs. Same with the display none and block in all three functions. Is there a better way to do this?
Again, Thank you for taking your valuable time to review this. I have learned some things new here. I was reluctant to submit solutions but this just encourages me to do so and get better. 😄
@mickyginger
Posted
@kashish-d Glad you learned something, but even more glad that I encouraged you. All this code stuff is complicated and the only way to get better it to keep on coding! 🙃
Sorry I missed the subtleties of the repeating code. I think that's another good reason to reduce repetition because it's easy to miss the little differences.
Luckily you can use event.target or event.currentTarget to establish the thing that triggered the event (ie the button that was clicked). So we can refactor the function like so:
const allTabs = document.querySelectorAll('.menu-item');
const allLists = document.querySelectorAll('.list-box');
const handleTabClick = (event) => {
// find the index of the tab within the array of tabs
const tabIndex = allTabs.indexOf(event.currentTarget);
// remove "active" class from *all* tabs
allTabs.forEach((tab) => tab.classList.remove('active'));
// hide all lists
allLists.forEach((list) => list.style.display = 'none');
// add "active" class to clicked tab
event.currentTarget.classList.add('active');
// display relevant list
allLists[tabIndex].style.display = 'block';
}
You'll need to add the class of "list-box" to your lists in your HTML:
<div class="list-box all-list-box">...</div>
<div class="list-box active-list-box">...</div>
<div class="list-box completed-list-box">...</div>
The key thing here is establishing which tab was clicked and then which list relates to that tab. The simplest way of doing that with the code you have is to find out the index of the tab that was clicked and find the list by the same index. That means the tabs and the lists have to be in the same order. Here's some info on indexOf if you wanna take a look.
Another way of linking the list to the tab would be to use a data attribute to explicitly set the class or id of the list you want to activate. Something like:
<li class="menu-item active" data-listid="all-list-box">...</li>
<li class="menu-item" data-listid="active-list-box">...</li>
<li class="menu-item" data-listid="completed-list-box">...</li>
Now you can target the list explicitly in your JavaScript and you don't have to keep all the tabs and the lists in the right order:
const handleTabClick = (event) => {
// remove "active" class from *all* tabs
allTabs.forEach((tab) => tab.classList.remove('active'));
// hide all lists
allLists.forEach((list) => list.style.display = 'none');
// add "active" class to clicked tab
event.currentTarget.classList.add('active');
// get list id from data attribute
const listId = event.currentTarget.dataset.listid;
// find the list by id
const selectedList = document.getElementById(listId);
// display relevant list
selectedList.style.display = 'block';
}
I'm using ids here, so you'd need to update your lists like so:
<div class="list-box" id="all-list-box">...</div>
<div class="list-box" id="active-list-box">...</div>
<div class="list-box" id="completed-list-box">...</div>
Ok, that's all from me, I hope it's helpful! 🤓
Marked as helpful
@kashish-d
Posted
@mickyginger Oh, That's awesome. I really like the idea of using data attributes! A little more than array one. Thank you so much for the help, Mike. Everything in detail made it so easy to understand. I really appreciate this!! 😊 As it was so confusing, I will definitely try to avoid violating DRY!
