Skip to content

fix small bags and code clearing#2

Open
andrsvsrg wants to merge 12 commits into
masterfrom
copyForComment
Open

fix small bags and code clearing#2
andrsvsrg wants to merge 12 commits into
masterfrom
copyForComment

Conversation

@andrsvsrg
Copy link
Copy Markdown
Owner

No description provided.

Comment thread src/App.js Outdated

return (
<div className="app">
<Header
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

fix

Comment thread src/components/Header/Header.js Outdated
import deleteDayTasksSVG from "../../icon/deleteDayTasks.svg";
import deleteAllTasksSVG from "../../icon/deleteAllTasks.svg";

const Header = React.memo(function Header({
Copy link
Copy Markdown

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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

fix

Comment thread src/components/UI/button/MyButton.js Outdated

const MyButton = (props) => {
return (
<button { ...props } className={ "myButton-default-style " + props.className }>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

файл назвать index.js
className выносим за return

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

fix

Comment thread src/components/Header/Header.js Outdated
height="25px"
/>
</MyButton>
<ReactTooltip id="deleteThisDay" type="error" effect="solid">
Copy link
Copy Markdown

@viktor-liablin viktor-liablin Aug 21, 2022

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"
>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

fix

Comment thread src/App.js Outdated

function App() {
const [todo, setToDo] = useState(
() => JSON.parse(localStorage.getItem("items")) || {}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

одинарные кавычки везде кроме когда в пропсы передаем строку, точка с запитаю нигде не нужно только в каких то блоках for (i =0 ; ...)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

fix

Comment thread src/components/ToDoList/ToDoList.js Outdated
updateTaskPosition,
changeTaskTitle,
}) {
const selectedDayTasksId = useMemo(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ты уже это делал в Parent, можно было об оттуда передать, но я говорил уже, лучше б это было так
selectedDay.id

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Прокинул через пропсы. fix

Comment thread src/components/ToDoList/ToDoList.js Outdated
}}
>
<div className="draggable-element">
<ToDoItem
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Если ToDoItem используется всегда с draging, то Draggable туда можно поместить сразу, чтоб в этом файле был только ToDoItem

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

fix

Comment thread src/App.js Outdated
setToDo({ ...todo, [selectedDayTasksId]: copySelectDayToDo });
}

function completedTask(task) {
Copy link
Copy Markdown

@viktor-liablin viktor-liablin Aug 21, 2022

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: 'Моя задача' }

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Одна функция вместо 3х)) fix

Comment thread src/App.js Outdated
function completedTask(task) {
const copySelectDayToDo = todo[selectedDayTasksId].map((element) => {
if (element.id === task.id) {
element.isCompleted = !element.isCompleted;
Copy link
Copy Markdown

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 }
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Да ошибка, поверхностно думал что мар вернет новый массив , но вложенные обьекты то останутся привязаными и будут мутироваться. Функция удалена, новая updateTask

Comment thread src/App.js Outdated
setToDo({ ...todo, [selectedDayTasksId]: copySelectDayToDo });
}

function updateTaskPosition(data, index) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

почему index, у таски вроде id есть

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants