Welcome to the Treehouse Community
Want to collaborate on code errors? Have bugs you need feedback on? Looking for an extra set of eyes on your latest project? Get support with fellow developers, designers, and programmers of all backgrounds and skill levels here with the Treehouse Community! While you're at it, check out some resources Treehouse students have shared here.
Looking to learn something new?
Treehouse offers a seven day free trial for new students. Get access to thousands of hours of content and join thousands of Treehouse students and alumni in the community today.
Start your free trialGrzegorz Zielinski
5,838 PointsImproved solution for buttons challenge - what do you think?
Ok, I know it's been a while but I've made some more improvements to my JS code yesterday following your advice, and using a new skills I've learned meanwhile (callbacks) :)
Here is my new improved solution, I wonder what you think about it:
const descriptionInput = document.querySelector("input.description");
const descriptionP = document.querySelector("p.description");
const descriptionButton = document.querySelector("button.description");
const toggleList = document.querySelector("#toggleList");
const list = document.querySelector(".list");
const ulList = list.querySelector("ul");
const addItemInput = document.querySelector("input.addItemInput");
const addItemButton = document.querySelector("button.addItemButton");
const firstListElement = document.querySelector(".up");
const lis = ulList.children;
const firstPosition = ulList.firstElementChild;
const lastPosition = ulList.lastElementChild;
// My part of code starts here
function attachListItemButtons(li) { // checks if element is first/last element of ulList and then - if not, attaches all the buttons, if some of the condition is true - skips that part of a function. Also checks if "li" doesn't have "null" valule which is useful if we remove all the elements and it's only one left on page.
if (li !== null) {
if (li !== ulList.firstElementChild) {
let up = document.createElement("button");
up.className = "up";
up.textContent = "Up";
li.appendChild(up);
}
if (li !== ulList.lastElementChild) {
let down = document.createElement("button");
down.className = "down";
down.textContent = "Down";
li.appendChild(down);
}
let remove = document.createElement("button");
remove.className = "remove";
remove.textContent = "Remove";
li.appendChild(remove);
}
}
function firstLastBGColor() { //Same as before - gives different colors to last/first element
let first = ulList.firstElementChild;
let last = ulList.lastElementChild;
for (let i = 0; i < lis.length; i += 1) {
lis[i].style.background = "white";
first.style.background = "lightblue";
last.style.background = "lightpink";
ulList.firstElementChild.firstElementChild.style.background = "#508abc";
}
}
function removeButtons(element) { // removes all the buttons from "element" and also checks if it doesn't have "null" value (same reason as above with "attachListItemButtons"
if (element !== null) {
var child = element.lastElementChild;
while (child) {
element.removeChild(child);
child = element.lastElementChild;
}
}
}
function removeAndAttach(func1, func2) { // callback which uses two functions and simple loop for 2nd, 3rd, 4th, and 5th arument
for (i = 2; i < 6; i++) {
func1(arguments[i]);
func2(arguments[i]);
}
}
for (let i = 0; i < lis.length; i += 1) { // generates all the buttons when the page loads for first time
attachListItemButtons(lis[i]);
}
firstLastBGColor(); // generate colors when the page loads for first time
list.addEventListener("click", (event) => { // event which triggers functions below
let first = ulList.firstElementChild; // variables aiming first/second/penultimate/last element on page
let second = ulList.firstElementChild.nextElementSibling;
let penultimate = ulList.lastElementChild.previousElementSibling;
let last = ulList.lastElementChild;
removeAndAttach( // callback function used here to first remove and then to attach new buttons to each of argument.
removeButtons,
attachListItemButtons,
first,
second,
penultimate,
last
);
firstLastBGColor();
});
// End of my part of code
ulList.addEventListener("click", (event) => {
if (event.target.tagName == "BUTTON") {
if (event.target.className == "remove") {
let li = event.target.parentNode;
let ul = li.parentNode;
ul.removeChild(li);
}
if (event.target.className == "up") {
let li = event.target.parentNode;
let prevLi = li.previousElementSibling;
let ul = li.parentNode;
if (prevLi) {
ul.insertBefore(li, prevLi);
}
}
if (event.target.className == "down") {
let li = event.target.parentNode;
let nextLi = li.nextElementSibling;
let ul = li.parentNode;
if (nextLi) {
ul.insertBefore(nextLi, li);
}
}
}
});
descriptionButton.addEventListener("click", () => {
descriptionP.innerHTML = `<h2>${descriptionInput.value}:</h2>`;
descriptionInput.value = "";
});
toggleList.addEventListener("click", () => {
if (list.style.display == "none") {
list.style.display = "block";
toggleList.textContent = "Hide List";
} else {
list.style.display = "none";
toggleList.textContent = "Show List";
}
});
addItemButton.addEventListener("click", () => {
let ul = document.querySelector("ul");
let li = document.createElement("li");
li.textContent = addItemInput.value;
attachListItemButtons(li);
ul.appendChild(li);
addItemInput.value = "";
});
I know there is still one more thing I could improve - when there is less than 4 elements in ulList removeAndAttach callback function removes and attach buttons to same elements twice (when for the example there is 3 elements then 2nd one is taken as a "second " and "penultimate" variable so in this case buttons are removed->generated->removed->generated again) , but I need more time to sort it out, so that's going to be my next task :)
Have you got any other advice for me, or ideas for improvements?
Thanks!
1 Answer
Tobias Edwards
14,458 PointsYou are completely right in what you say:
At the moment, you are always removing and then re-adding the buttons for the top two and bottom two list elements. This is better than refreshing the buttons for all list items, but if the list is huge, then it is unlikely a refresh is needed because most movement will take place in the middle of the list.
Creating and removing elements is relatively costly, so I would chuck in a few extra if-statements to reduce the number of element creation/deletions.
Another post mentions disabling/enabling the buttons, so you could use that instead and avoid creating/removing elements altogether. You might need to add a CSS class that can be toggled to clearly indicate the button is disabled.
Some of the coding style can be improved e.g. the removeAndAttach
function is always called the same way: passing in two functions, and then arguments to act on. Would be better to just move the removeButtons
and attachListButtons
functions inside removeAndAttach
, and then call the two functions on the arguments:
function refreshListButtons() {
for (let i = 0; i < arguments.length; i++) {
const li = arguments[i];
removeButtons(li);
attachListButtons(li);
}
}
Something interesting: Notice how moving items up and down only visually changes the text item positions i.e. the actual buttons are visually static in position. Behind the scenes, at the moment, list items (text and their buttons) are being moved together as one. It would be interesting to detach the buttons from their associated text, and move the text elements up and down only so the buttons always stay the same (no button creation/deletion at all!). This would mean each "Up" and "Down" button press would require finding its position n, and then moving the n-th text item... Probably more work than it is worth though ha ha