feat update: work began #2

Ouriel91 · 2022-05-15T15:21:30Z

No description provided.

SergeiSafrigin · 2022-05-26T13:00:40Z

src/ex1/index.html

+                <option value="Z-A">Z-A</option>
+            </select>
+        </div>
+        <div class="input-container">


Great layout modeling! well done!

SergeiSafrigin · 2022-05-26T13:08:36Z

src/ex1/style.css

+
+* {
+    box-sizing: border-box;
+    margin: 0;


in general, it's usually a bad practice to use the wildcard selector *, as it applies to all elements, rather than inheriting the style from a parent element (it still could be html or body) which are much easier to override when needed.
You can read more on CSS precedence here - https://jenkov.com/tutorials/css/precedence.html

You also have this cool library that normalizes the base/default styles across different browsers - https://necolas.github.io/normalize.css/

SergeiSafrigin · 2022-05-26T13:15:35Z

src/ex1/style.css

+
+body {
+    width: 100%;
+    height: 100vh;


in order for the viewport styles (vh/vw) to apply correctly you need to add this meta tag to the HTML header

You can read more on css viewport here https://www.educba.com/css-viewport/

SergeiSafrigin · 2022-05-26T13:18:43Z

src/ex1/style.css

+.add-button {
+    width: 10%;
+    min-width: 35px;
+    height: 100%;


it's a good practice to align the base style of different components such as "button", to avoid code duplication, improve maintainability & have a consistent UI across the app.
You can add a button class with that base style and apply it to all the buttons.

(same goes for input and other components)

SergeiSafrigin · 2022-05-26T13:19:09Z

src/ex1/style.css

+    transition: all 0.3s ease;
+}
+
+.add-button.active, .clear-all-button.active {


nice re-use!

SergeiSafrigin · 2022-05-26T13:24:42Z

src/ex1/style.css

+    position: relative;
+    background-image: url("https://www.itgovernance.eu/blog/de/wp-content/uploads/2017/04/list.jpg");
+    background-size: cover;
+    height: 40vh;


viewport styles are considered a bad practice when used on inner components as it ignores the 'box' it's inside of, and uses the whole viewport as a reference, making it hard to maintain and forces you to change its size anytime the parent UI changes.

SergeiSafrigin · 2022-05-26T13:26:17Z

src/ex1/style.css

+    top: 50%;
+    left: 50%;
+    transform: translateX(-50%) translateY(-50%);
+    z-index: 100;


Much easier to achieve with flex-box and is considered a better practice in general - https://css-tricks.com/snippets/css/a-guide-to-flexbox/

SergeiSafrigin · 2022-05-26T13:26:54Z

src/ex1/style.css

+
+.todo-list-container {
+    max-height: 250px;
+    overflow-y: auto;


SergeiSafrigin · 2022-05-26T13:29:31Z

src/ex1/style.css

+    overflow-y: auto;
+}
+
+.todo-list-container li {


Using "li" here instead of a class makes it coupled to this specific solution. If you'll need to change li to a different element (ex: div) you'll have to make many changes in the code

SergeiSafrigin · 2022-05-26T13:31:20Z

src/ex1/style.css

+    position: absolute;
+    right: -40px;
+    top: 50%;
+    transform: translateY(-50%);


the positioning here is very hard to maintain, as every change of the width will require adjusting the position of all the elements, flex-box will help you here as well

SergeiSafrigin · 2022-05-26T13:37:30Z

src/ex1/script.js

+const clearAllTodosButton = document.getElementById("clear-all-todos-button")
+const sumTodos = document.getElementById("sum-todos")
+const orderSelect = document.getElementById("order-select")
+const enterTodos = document.getElementById("enter-todos")


having all the elements outside here requires all the dom elements 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

SergeiSafrigin · 2022-05-26T13:37:46Z

src/ex1/script.js

+
+showTodos()
+
+todoInput.onkeyup = (e) => {


it's a good practice to use the addEventListener method to add dom events

SergeiSafrigin · 2022-05-26T13:38:39Z

src/ex1/script.js

+
+todoInput.onkeyup = (e) => {
+    let enterValue = todoInput.value
+    if (enterValue.trim() !== ""){


well done with this extra validation!

SergeiSafrigin · 2022-05-26T13:40:42Z

src/ex1/script.js

+        addTodoButton.style.cursor = "pointer"
+        addTodoButton.style.opacity = 1
+
+        if(e.keyCode === 13){


try to avoid "magic numbers", creating a const for this value will improve readability, for example:

const ENTER_KEY = 13; // at the top of the file

if (e.keyCode === ENTER_KEY) {...}

SergeiSafrigin · 2022-05-26T13:44:11Z

src/ex1/script.js

+        }
+    }
+    else{
+        addTodoButton.classList.remove("active")


it's considered a good practice to hide the implementation of different actions to improve code readability, maintability & code-reuse, for example:

const isEmptyTodoValue = e.keyCode === ENTER_KEY;
if (isEmptyTodoValue) {
deactivateAddTaskBtn();
} else {
activateAddTaskBtn();
if (e.keyCode === ENTER_KEY) {
addTodo();
}
}

SergeiSafrigin · 2022-05-26T13:46:37Z

src/ex1/script.js

+function addTodo(){
+    let enterValue = todoInput.value
+
+    if(enterValue.trim() === ""){


the second time you check this -> could be moved to a function

SergeiSafrigin · 2022-05-26T13:48:14Z

src/ex1/script.js

+        return
+    }
+
+    checkIfExistDataFromLS()


I wonder why we have this check here as well

SergeiSafrigin · 2022-05-26T13:51:20Z

src/ex1/script.js

+        todoList = []
+    }
+    else{
+        todoList = JSON.parse(dataFromLS)


it's a bad practice to update some global variable from within a function, better use return instead :)

SergeiSafrigin · 2022-05-26T13:51:38Z

src/ex1/script.js

+    }
+
+    checkIfExistDataFromLS()
+    pushEnteredDataToLS(enterValue)


Awesome! love it!!!

Small bug: the enterValue is not trimmed before it's added

SergeiSafrigin · 2022-05-26T13:53:27Z

src/ex1/script.js

+}
+
+function showTodos() {
+    checkIfExistDataFromLS()


why don't we do this only once when the app loads?

SergeiSafrigin · 2022-05-26T13:56:49Z

src/ex1/script.js

+
+orderSelect.addEventListener('change', (e) => {
+    let dataFromLS = localStorage.getItem("new-todo")
+    todoList = JSON.parse(dataFromLS)


it's better to encapsulate all the local storage operations behind functions to avoid accessing it directly. Then you could also build wrappers that both fetch and save the todos, and handle that calls to the local storage functions when needed

SergeiSafrigin

See comments

SergeiSafrigin · 2022-05-26T14:00:48Z

src/ex1/script.js

+            <span class="delete" onclick="deleteTodo(${index})";>
+                <i class="fas fa-trash"></i>
+            </span>
+            <span class="edit" onclick="editTodo(${index})";>


functions inside the html are considered a bad practice and can lead to many bugs, prefer using addEventListener

SergeiSafrigin · 2022-05-26T14:01:35Z

src/ex1/script.js

+            `
+    })
+
+    todoListElement.innerHTML = listItems


cool idea!!! but has some downsides as you can't access the elements while creating them before they enter the dom

SergeiSafrigin · 2022-05-26T14:02:13Z

src/ex1/script.js

+}
+
+function editTodo(index) {
+    let editedValue = prompt('Edit todo:', todoList[index])


BUG: if you cancel the edit, it will rename it to null

SergeiSafrigin · 2022-05-27T12:24:13Z

src/ex1/script.js

+    todoList = JSON.parse(dataFromLS)
+
+    if(e.target.value === "A-Z") {
+        todoList = todoList.sort()


the sort method mutates the original array, therefore it's better to copy it beforehand ex: [...todoList.sort()];

As mutating the state directly (and complex data structures in general) is considered a bad practice as other parts of the code might be using it, not expecting it to suddenly change.
Read more on pure functions and the immutability approach in coding https://css-tricks.com/understanding-immutability-in-javascript/

Hi @SergeiSafrigin i fixed all the comments that you gave to me it was very helpfull

Ouriel91 added 6 commits May 15, 2022 18:20

feat update: work began

3fd4239

feat update: adds application main style

4a9cb95

feat update adds the main logic of todolist

fde52a4

feat update: added a-z and z-a filter

eee5951

feat update: adds edit option

6ea658f

feat update: addsempty state

fae702d

orimaromcom mentioned this pull request May 19, 2022

Ex1 orimaromcom/monday-u-exercises#1

Merged

SergeiSafrigin reviewed May 26, 2022

View reviewed changes

src/ex1/style.css

.todo-list-container {

max-height: 250px;

overflow-y: auto;

Copy link

SergeiSafrigin May 26, 2022

Choose a reason for hiding this comment

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

like!

Ouriel91 reacted with thumbs up emoji

SergeiSafrigin reviewed May 26, 2022

View reviewed changes

SergeiSafrigin suggested changes May 26, 2022

View reviewed changes

SergeiSafrigin reviewed May 26, 2022

View reviewed changes

SergeiSafrigin reviewed May 27, 2022

View reviewed changes

Ouriel91 closed this May 31, 2022

Ouriel91 reopened this May 31, 2022

feat update: work began #2

Are you sure you want to change the base?

feat update: work began #2

Uh oh!

Conversation

Ouriel91 commented May 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeiSafrigin May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeiSafrigin May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeiSafrigin May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeiSafrigin May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeiSafrigin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeiSafrigin May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SergeiSafrigin May 26, 2022 •

edited

Loading

SergeiSafrigin May 26, 2022 •

edited

Loading

SergeiSafrigin May 26, 2022 •

edited

Loading

SergeiSafrigin May 26, 2022 •

edited

Loading

SergeiSafrigin May 26, 2022 •

edited

Loading