-
Notifications
You must be signed in to change notification settings - Fork 0
fix small bags and code clearing #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
src/App.js
Outdated
|
|
||
| return ( | ||
| <div className="app"> | ||
| <Header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Header в Ract memo, в него передаются функции deleteSelectedDayAllTasks, deleteAllTasks которые создаются при каждом рендере App. То-есть в мемо каждый раз новые функции приходят, он думает что-то то изменилось и рендерит каждый раз, нет смысла юзать memo тогда. ЧТоб функции не создавались каждый раз, нужен useCallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
src/components/Header/Header.js
Outdated
| import deleteDayTasksSVG from "../../icon/deleteDayTasks.svg"; | ||
| import deleteAllTasksSVG from "../../icon/deleteAllTasks.svg"; | ||
|
|
||
| const Header = React.memo(function Header({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function Header (props) {
...
}
export default React.memo(Header)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
src/components/UI/button/MyButton.js
Outdated
|
|
||
| const MyButton = (props) => { | ||
| return ( | ||
| <button { ...props } className={ "myButton-default-style " + props.className }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
файл назвать index.js
className выносим за return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
src/components/Header/Header.js
Outdated
| height="25px" | ||
| /> | ||
| </MyButton> | ||
| <ReactTooltip id="deleteThisDay" type="error" effect="solid"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В MyButton можно добавить возможность работы с тултипом
MyButtn
data-tip
data-for="deleteThisDay"
onClick={deleteSelectedDayAllTasks}
className="header-buttons"
tooltip="Delete tasks from this day"
>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
src/App.js
Outdated
|
|
||
| function App() { | ||
| const [todo, setToDo] = useState( | ||
| () => JSON.parse(localStorage.getItem("items")) || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
одинарные кавычки везде кроме когда в пропсы передаем строку, точка с запитаю нигде не нужно только в каких то блоках for (i =0 ; ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
src/components/ToDoList/ToDoList.js
Outdated
| updateTaskPosition, | ||
| changeTaskTitle, | ||
| }) { | ||
| const selectedDayTasksId = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ты уже это делал в Parent, можно было об оттуда передать, но я говорил уже, лучше б это было так
selectedDay.id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Прокинул через пропсы. fix
src/components/ToDoList/ToDoList.js
Outdated
| }} | ||
| > | ||
| <div className="draggable-element"> | ||
| <ToDoItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если ToDoItem используется всегда с draging, то Draggable туда можно поместить сразу, чтоб в этом файле был только ToDoItem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
src/App.js
Outdated
| setToDo({ ...todo, [selectedDayTasksId]: copySelectDayToDo }); | ||
| } | ||
|
|
||
| function completedTask(task) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не понятно что функция по названию делает, нет глагола
markTaskAsCompleted
Но тут то еще она не только делает ее завершенной но и наоборот
changeTaskCompletedStatus
Но вообще это все про апдейт task
completedTask, updateTaskPosition, changeTaskTitle
Нужны просто функция updateTask(taskId, changes)
changes это обьект { title: 'Моя задача' }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Одна функция вместо 3х)) fix
src/App.js
Outdated
| function completedTask(task) { | ||
| const copySelectDayToDo = todo[selectedDayTasksId].map((element) => { | ||
| if (element.id === task.id) { | ||
| element.isCompleted = !element.isCompleted; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нельзя так делать, ты идешь по таскам и мутируешь уже существующий element, а должен возвращатся новый построенной на инфе из element
if (element.id === task.id) {
return { ...element, isCompleted: !element.isCompleted }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да ошибка, поверхностно думал что мар вернет новый массив , но вложенные обьекты то останутся привязаными и будут мутироваться. Функция удалена, новая updateTask
src/App.js
Outdated
| setToDo({ ...todo, [selectedDayTasksId]: copySelectDayToDo }); | ||
| } | ||
|
|
||
| function updateTaskPosition(data, index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему index, у таски вроде id есть
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
No description provided.