Exercise 1 #4
Exercise 1 #4
Conversation
| .task-input-wrapper { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| margin-bottom: 15px; |
There was a problem hiding this comment.
As the relative positioning goes from left to right and top to bottom, it's a better practice to control the element's position against the existing dom on the top & left, thus using margin-left/margin-top instead of positioning the next set of elements (right-bottom).
| } | ||
|
|
||
| .task-button { | ||
| margin-right: 25px; |
There was a problem hiding this comment.
good job on reusing the button class!
| position: absolute; | ||
| width: 35%; | ||
| left: 25px; | ||
| top: 540px; |
There was a problem hiding this comment.
The positioning here (absolute against the whole page) will be difficult to maintain, as almost any change to the layout will require adjusting the positions. look for other ways to position the footer part of the app
| Can't wait when you add your first task! | ||
| </h2> | ||
| </ul> | ||
| <button id="sort-by-name-button" class="task-button hidden"> |
There was a problem hiding this comment.
you might want to wrap the sort & clear all buttons with a footer container to easier positioning
| const addTaskButton = document.querySelector(".task-button"); | ||
| const addTaskInput = document.querySelector(".task-input"); | ||
| const clearAllButton = document.querySelector("#clear-all-button"); | ||
| const sortByNameButton = document.querySelector("#sort-by-name-button"); |
There was a problem hiding this comment.
having all the elements outside here requires all the dom elements to exist when this file is loaded, which might not be the case in complex web apps that dynamically change the dom depending on the view
| <body> | ||
| <div class="white-box"> | ||
| <h1 class="main-header">Task Manager</h1> | ||
| <div class="task-input-wrapper"> |
| </button> | ||
| </div> | ||
| <ul class="task-list"> | ||
| <div id="triangle" class="triangle"></div> |
There was a problem hiding this comment.
The "ul" should only have "li" direct children
|
|
||
| function hideWelcomePart() { | ||
| addButton.classList.remove("hithere"); | ||
| clearAllButton.classList.remove("hidden"); |
There was a problem hiding this comment.
Moving the welcome page into its own container (maybe with a shared class for some of the styles) will make your life easier here - 1 line of code instead of 6. Also in the current state, every new element you'll add to the task-list container will require you to handle it here as well
| function showWelcomePart() { | ||
| addButton.classList.add("hithere"); | ||
| clearAllButton.classList.add("hidden"); | ||
| sortByNameButton.classList.add("hidden"); |
| liElm.addEventListener("click", () => alert(`Task: ${text}`)); | ||
|
|
||
| //clear input | ||
| document.querySelector(".task-input").value = ""; |
There was a problem hiding this comment.
could be moved into it's own function
| //add ids | ||
| const idText = text.split(" ").join("-"); | ||
| divElm.setAttribute("id", `${idText}`); | ||
| deleteButton.setAttribute("id", `${idText}-button`); |
There was a problem hiding this comment.
can be dangerous as these ids are not unique.
|
|
||
| //remove <div> with task | ||
| const divElementId = buttonId.join("-"); | ||
| document.querySelector(`#${divElementId}`).remove(); |
There was a problem hiding this comment.
consider eaither using unique ids for the tasks. You could use UUID or traverse the dom from the target up until you find the task (target.parentElement)
| const elementIndex = listOfTasks.findIndex((task) => { | ||
| return task === taskText; | ||
| }); | ||
| listOfTasks.splice(elementIndex, 1); |
There was a problem hiding this comment.
Bug: removing a task with a not unique name might not remove the one the user wanted. Consider making each task a complex object with id and text properties.
| } | ||
|
|
||
| function sortByName() { | ||
| listOfTasks.forEach((task) => { |
There was a problem hiding this comment.
duplication, consider moving it into it's own function
| listOfTasks.sort(); | ||
| console.log(listOfTasks); | ||
|
|
||
| listOfTasks.forEach((task) => { |
| } | ||
|
|
||
| function capitalize(string) { | ||
| const updatedString = string.charAt(0).toUpperCase() + string.slice(1); |
| } | ||
|
|
||
| function createTask(text) { | ||
| //create elements |
There was a problem hiding this comment.
good job on the separation of concerns between the functions!
|
|
||
| .task-list { | ||
| display: flex; | ||
| flex-direction: column; |
There was a problem hiding this comment.
Visual bug: There is no scroll, so if I add too many tasks it overflows
No description provided.