PR to add JavaScript ToDo app #1
Conversation
debarshi/js/components/todoBase.js
Outdated
| const onTodoDragStart = (event) => { | ||
| const dataId = event.currentTarget.getAttribute("data-id") | ||
| event.dataTransfer.dropEffect = "move" | ||
| event.dataTransfer.setData("data_id", dataId) |
There was a problem hiding this comment.
two different variable with same name but different convention would be confusing - data-id and data_id
debarshi/js/components/todoBase.js
Outdated
| ) | ||
|
|
||
| if (currentTodoIdx >= 0 && previousTodoIdx >= 0) { | ||
| const temp = todos[currentTodoIdx] |
There was a problem hiding this comment.
can use destructing assignment here for swapping to avoid temp - Eg: [b,a] = [a,b]
debarshi/js/components/todoBase.js
Outdated
| import { todoWrapper } from "../selectors.js" | ||
|
|
||
| const createTodoBase = (todo, props = {}) => { | ||
| const { draggable = false } = props |
There was a problem hiding this comment.
should use semicolons after every statement
debarshi/js/components/index.js
Outdated
| @@ -0,0 +1,5 @@ | |||
| export { default as createTodoBase } from "./todoBase.js" | |||
| export { default as createTodoRoot } from "./todoRoot.js" | |||
There was a problem hiding this comment.
should use semicolon after every statement. Applies to all the files.
debarshi/js/todo.js
Outdated
| case "clear": | ||
| return clearAllTodos() | ||
| case "mark-complete": | ||
| return handleMarkAllComplete() |
There was a problem hiding this comment.
linter might give warning for not having default case. Also i suggest using object mapping (eg: {'add-sub-todo' : handleAddTodo}) instead of switch case if possible.
debarshi/js/utils/helperfuncs.js
Outdated
| @@ -0,0 +1,65 @@ | |||
| const validateInput = (value = "") => { | |||
There was a problem hiding this comment.
file naming doesn't seem correct. should be helpers.js/utils.js etc but not helpersfuncs.js since helpersfuns is neither snake case or any other standard naming convention
debarshi/js/utils/helperfuncs.js
Outdated
| const findTodo = (todos = [], todoId) => { | ||
| let searchedTodo | ||
|
|
||
| for (let todo of todos) { |
debarshi/js/utils/helperfuncs.js
Outdated
| } | ||
|
|
||
| const findNode = (parentNode = {}, tagName = "") => { | ||
| let resultNode |
There was a problem hiding this comment.
seems an incomplete statement without semicolon
Debarshi95
force-pushed
the
user/debarshi-todos
branch
from
09488d6 to
0fa4d58
Compare
PR to add JavaScript training ToDo app
Changes:
Functionality:
Demo:
Demo
Screenshots:
