-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Develop #1534
base: master
Are you sure you want to change the base?
Develop #1534
Conversation
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.
GJ 👍 It works perfect. Added comments to refactor your code and make it better
src/App.tsx
Outdated
onSubmit={handleSubmit} | ||
loading={loading} | ||
title={title} | ||
onTitleChange={value => setTitle(value)} |
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.
onTitleChange={value => setTitle(value)} | |
onTitleChange={setTitle} |
src/components/TodoList/TodoList.tsx
Outdated
arg1: React.KeyboardEvent<HTMLInputElement>, | ||
arg2: React.Dispatch<React.SetStateAction<boolean>>, | ||
) => void; | ||
processedIs: number[]; |
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.
processedIs: number[]; | |
processedIds: number[]; |
typo - fix everywhere
src/components/TodoItem/TodoItem.tsx
Outdated
onItemTitleUpdate( | ||
id, | ||
newTitle, | ||
setTitleBeingUpdated, |
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.
no need to pass this function as a callback
Just create onBlur
function here and call inputRef.current.focus();
there
src/components/TodoItem/TodoItem.tsx
Outdated
isTitleChanged, | ||
) | ||
} | ||
onKeyUp={event => onKeyUp(event, setTitleBeingUpdated)} |
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.
same - remove setTitleBeingUpdated
state at all
src/App.tsx
Outdated
setTitleBeingUpdated: React.Dispatch<React.SetStateAction<boolean>>, | ||
isTitleChanged: boolean, | ||
e?: React.FormEvent<HTMLFormElement>, |
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.
do not pass it as an arguments
Instead, in TodoItem component create a separate function handler onSubmit
. In this function, you should apply local logic (event.preventDefault, input focus, etc.) and just call this function with todo id and new title
Apply the same approach for other functions as well
src/components/Footer/Fotter.tsx
Outdated
type Props = { | ||
todos: Todo[]; | ||
status: TodoStatus; | ||
onStatusChange: (arg: TodoStatus) => void; |
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.
onStatusChange: (arg: TodoStatus) => void; | |
onStatusChange: (status: TodoStatus) => void; |
arg
is a bad name, even in types
src/components/Footer/Fotter.tsx
Outdated
}) => { | ||
const activeTodos = todos.filter(todo => !todo.completed); | ||
const isAnyCompleted = todos.some(todo => todo.completed); | ||
const filterOptions = Object.values(TodoStatus); |
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.
nit: you can move this variable outside of the component
src/components/Footer/Fotter.tsx
Outdated
const isAnyCompleted = todos.some(todo => todo.completed); | ||
const filterOptions = Object.values(TodoStatus); | ||
|
||
const capitalizeFirstLetter = (value: TodoStatus) => { |
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.
Same - you can create this function outside of the component (or even in a separate helpers.ts file and import it here)
src/components/TodoItem/TodoItem.tsx
Outdated
title: string; | ||
onItemDelete: (id: number) => void; | ||
onItemStatusUpdate: (id: number) => void; | ||
onItemTitleUpdate: ( |
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.
you can simplify this function (see other comments)
Also, write correct names for arguments
src/App.tsx
Outdated
.catch(() => setErrorMessage('Unable to load todos')); | ||
}, []); | ||
|
||
const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { |
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.
Do not forget about useCallback
and useMemo
for functions and values
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.
got you!) |
DEMO LINK