Exercises-1 pull req Dan Menahem #6

DanMenahem · 2022-05-18T16:49:07Z

No description provided.

…ture

… is deleted it alert

SergeiSafrigin · 2022-05-27T11:46:46Z

src/ex1/index.html

+        </div>
+        <div id="info">
+            <span id="peding-task-text">You have 0 pending tasks</span>
+            <button class="btn" id="clearAllBtn" disabled>Clear All</button>


Good job on reusing the btn class!

SergeiSafrigin · 2022-05-27T11:47:37Z

src/ex1/style.css

+* {
+    font-family: Alef, Arial, Helvetica, sans-serif;
+    font-size: 20px;
+    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-27T11:51:51Z

src/ex1/style.css

+.btn[disabled] {
+    opacity: 0.3;
+    box-shadow: none;
+    border: none;


duplicate? isn't it already in .btn?

SergeiSafrigin · 2022-05-27T11:52:02Z

src/ex1/style.css

+
+.btn[disabled]:hover {
+    cursor: not-allowed;
+    box-shadow: none;


duplicate? isn't it already in .btn[disabled]? (move below .btn:hover)

SergeiSafrigin · 2022-05-27T11:54:57Z

src/ex1/style.css

+    box-shadow: none;
+}
+
+.btn {


it's a good practice to put all the different variants of a style after the base is defined so the variants will get a priority and could override the base class with ease and without the need for !important

SergeiSafrigin · 2022-05-27T11:56:52Z

src/ex1/style.css

+}
+
+
+#taskList 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-27T12:02:39Z

src/ex1/style.css

+    overflow: hidden;
+}
+
+#taskList li span {


The same goes for the span here better to use a class name. Also for readability, a descriptive classname will tell me on which element this style is targeting

SergeiSafrigin · 2022-05-27T12:05:03Z

src/ex1/script.js

+const addBtn = document.querySelector(" #addBtn ");
+const clearAllBtn = document.querySelector(" #clearAllBtn ");
+const sortAllBtn = document.querySelector(" #sortAllBtn ");
+const taskList = document.querySelector(" #taskList ");


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-27T12:13:44Z

src/ex1/script.js

+const placeHolder = document.querySelector(" .placeHolder ")
+const pedingTaskText = document.querySelector(" #peding-task-text ")
+
+const taskArr = JSON.parse(localStorage.getItem("tasks")) || [];


nice!
consider moving this to a function that handles the initial loading of the tasks

SergeiSafrigin · 2022-05-27T12:17:50Z

src/ex1/script.js

+
+
+//waiting for user input before enable the add button
+taskInput.addEventListener('keyup', () => addBtn.disabled = !taskInput.value)


extract the validation into a function as multiple sources need to use it :)

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

src/ex1/script.js

+
+//sort the tasks list add them to html list element and save them to the LocalStorage
+sortAllBtn.addEventListener("click", () => {
+    taskArr.sort();


the sort method mutates the original array, therefore it's better to copy it beforehand ex: taskArr = [...taskArr.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/

SergeiSafrigin · 2022-05-27T12:27:00Z

src/ex1/script.js

+
+//Clear the tasks list
+clearAllBtn.addEventListener("click", () => {
+    taskArr.length = 0;


not a common usage, I would change it to taskArr = [];
Also it mutates taskArr directly which is a bad practice due to the comment above

SergeiSafrigin · 2022-05-27T12:37:05Z

src/ex1/script.js

+//if there are some task in the localStorage insert them
+function init() {
+    if (taskArr.length != 0) insertAllTasks();
+    else placeHolder.classList.remove("hidden");


code duplication, this could have been instead a call to showEmptyState()/ showEmptyStateIfNeeded()/toggleEmptyState() or something similar

SergeiSafrigin · 2022-05-27T12:38:05Z

src/ex1/script.js

+}
+
+//Function to create the li element and its childs and assign a click event to them
+function createLi(taskValue) {


same as the css point, let's decouple this by changing to something more general like createTaskElement/createTaskNode

SergeiSafrigin · 2022-05-27T12:38:39Z

src/ex1/script.js

+}
+
+//if there are some task in the localStorage insert them
+function init() {


nice use of init, I would add the loading of the localStorage and initializing the tasksList here as well

SergeiSafrigin · 2022-05-27T12:41:39Z

src/ex1/script.js

+    i.classList.add("fa", "fa-trash-o");
+    span.appendChild(i);
+    li.appendChild(span);
+    span.onclick = (e) => {


prefer using the addEventListener instead of onclick directly

SergeiSafrigin · 2022-05-27T12:43:23Z

src/ex1/script.js

+}
+
+//update the button and the placeHolder if necessary
+function updateForm() {


Consider renaming the function to make it more descriptive or even separate into 2 different functions as you always know what you actually need to do beforehand. 1 for hiding the empty state and other for showing it

SergeiSafrigin · 2022-05-27T12:46:07Z

src/ex1/script.js

+
+// function to remove task that have been clicked
+function removeTask(li, taskValue) {
+    taskArr.splice(taskArr.indexOf(taskValue), 1);


As task holds only the text value it is not unique, therefore you might be removing the wrong task from the list. Consider an alternative approach. hint: find a way to uniquely identify tasks

SergeiSafrigin · 2022-05-27T12:48:16Z

src/ex1/index.html

-    <title>Exercise 1</title>
+    <meta charset="UTF-8">
+    <meta http-equiv="X-UA-Compatible" content="IE=edge">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">


SergeiSafrigin · 2022-05-27T12:48:51Z

src/ex1/index.html

+            <i class="fa fa-plus"></i>
+            <p>Time to add your first task</p>
+        </div>
+        <div id="info">


Good dom modeling, gj!

SergeiSafrigin

see comments

Ex2

feat: add fourth exercise

Ex3

Ex4

Ex5

DanMenahem and others added 13 commits May 14, 2022 11:26

update title and app name, and add icon

730004d

Add all the html elements and css

c9cc47b

Add all the functionality and fix some CSS and HTML

dae8f58

Change the shape of the functions for adding more functionality in fu…

b3aef8d

…ture

Change the way li are created and saved. fix the bug that when a task…

49f0f70

… is deleted it alert

Add animation when a new item is added

c291d5b

Edit some comments

d21409a

Merge branch 'monday-u-com:main' into main

f0f34a1

start Ex2

5569207

Refactor my current code to use classes with methods

de78f71

Add all the new class and the functionality

931ef53

fix a bug

9d3d245

delete console.log and rename a function

ddb4482

SergeiSafrigin reviewed May 27, 2022

View reviewed changes

src/ex1/style.css

.btn[disabled] {

opacity: 0.3;

box-shadow: none;

border: none;

Copy link

SergeiSafrigin May 27, 2022

Choose a reason for hiding this comment

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

duplicate? isn't it already in .btn?

SergeiSafrigin reviewed May 27, 2022

View reviewed changes

src/ex1/index.html

<i class="fa fa-plus"></i>

<p>Time to add your first task</p>

</div>

<div id="info">

Copy link

SergeiSafrigin May 27, 2022

Choose a reason for hiding this comment

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

Good dom modeling, gj!

SergeiSafrigin suggested changes May 27, 2022

View reviewed changes

DanMenahem and others added 19 commits May 30, 2022 11:24

Merge branch 'monday-u-com:main' into main

c230c99

refactor the code to work as cli

77ba7ef

add move the add todo function to ItemManager class

acf582e

fix the קxercise after the mentors comments

ffa366f

Merge pull request #1 from DanMenahem/Ex2

b0060ed

Ex2

add 'this.' before CheckIfEmpty function

4435ae9

add image with ascii art when pokemon added

e0e08c3

Merge pull request #3 from monday-u-com/main

aebf3e4

feat: add fourth exercise

change some comments

568035c

build a express backend and fix the client side

dec4ec5

Merge pull request #2 from DanMenahem/Ex3

415ce0a

Ex3

Initialize Sequelize and add items table

9b06aca

add status column and modify client and server to work with it

91c5900

Merge pull request #4 from DanMenahem/ex4

bc01042

Ex4

Merge pull request #5 from DanMenahem/ex5

84971ca

Ex5

Merge branch 'ex4'

1cc8c5d

Merge branch 'main' of https://github.com/DanMenahem/monday-u-exercise-1

94202eb

reset commit

2649797

Merge branch 'main' of https://github.com/DanMenahem/monday-u-exercise-1

cfe68e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exercises-1 pull req Dan Menahem #6

Exercises-1 pull req Dan Menahem #6

DanMenahem commented May 18, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022 •

edited

Loading

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022 •

edited

Loading

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022 •

edited

Loading

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin May 27, 2022

SergeiSafrigin left a comment



		//waiting for user input before enable the add button
		taskInput.addEventListener('keyup', () => addBtn.disabled = !taskInput.value)

Exercises-1 pull req Dan Menahem #6

Are you sure you want to change the base?

Exercises-1 pull req Dan Menahem #6

Conversation

DanMenahem commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeiSafrigin May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeiSafrigin May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeiSafrigin May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeiSafrigin left a comment

Choose a reason for hiding this comment

SergeiSafrigin May 27, 2022 •

edited

Loading

SergeiSafrigin May 27, 2022 •

edited

Loading

SergeiSafrigin May 27, 2022 •

edited

Loading