Develop #2930
Develop #2930
Conversation
src/App.tsx
Outdated
|
|
||
| const todosWithUser: TodoWithUser[] = todosFromServer.map( | ||
| (todo): TodoWithUser => { | ||
| const user = usersFromServer.find(({ id }) => id === todo.userId) as User; |
There was a problem hiding this comment.
I understand that this is for TS purposes.
But what if the user with the provided id is not found? Shit like that sometimes happens and we want to handle it instead of casting types
src/App.tsx
Outdated
| const [userId, setUserId] = useState(defaultUserId); | ||
| const [hasTitleError, setHasTitleError] = useState(false); | ||
| const [hasUserError, setHasUserError] = useState(false); | ||
| const isNotSelectedUser = userId === defaultUserId; |
There was a problem hiding this comment.
when we're dealing with booleans, we usually make the logic from positive, e.g. isSelectedUser, and use with NOT (!) operator.
It is difficult to read stuff like !isNotSelectedUser, since I have to compile in my mind what this variable means. In my mind it would sound like not is not selected user, this is difficult to read
src/App.tsx
Outdated
| const [hasUserError, setHasUserError] = useState(false); | ||
| const isNotSelectedUser = userId === defaultUserId; | ||
| const hasForbiddenSymbolsInTitle = !validateTitle(title); | ||
| const titleErrorMessage = hasForbiddenSymbolsInTitle |
There was a problem hiding this comment.
from this thing, it looks like there must be some error message anyway. Would be better to have it as a function, which can also return null if there's no error
|
|
||
| if (title && !isNotSelectedUser) { | ||
| const user = usersFromServer.find(({ id }) => id === +userId) as User; | ||
| const newId = Math.max(...todos.map(todo => todo.id)) + 1; |
There was a problem hiding this comment.
you don't have to use spread operator here, since map is not mutable
There was a problem hiding this comment.
Without this, I would have an error with the types. So maybe better to leave it to split the array to separate arguments for math max
src/App.tsx
Outdated
| } | ||
|
|
||
| if (title && !isNotSelectedUser) { | ||
| const user = usersFromServer.find(({ id }) => id === +userId) as User; |
There was a problem hiding this comment.
converting to a number with + is not a mistake, but not considered a clean approach. To make it easier to understand we usually convert the type explicitly, like Number(userId)
| userId: +userId, | ||
| id: newId, | ||
| title, | ||
| completed: false, |
There was a problem hiding this comment.
isCompleted would be a cleaner approach
src/App.tsx
Outdated
| setHasTitleError(false); | ||
| }} | ||
| /> | ||
| {hasTitleError && <span className="error">{titleErrorMessage}</span>} |
There was a problem hiding this comment.
this thing is difficult to read since there's a lot of stuff happening. There's a render condition, tag name, classname, text node.
Here's how I would approach it:
| {hasTitleError && <span className="error">{titleErrorMessage}</span>} | |
| {hasTitleError && ( | |
| <span className="error"> | |
| {titleErrorMessage} | |
| </span> | |
| )} |
src/App.tsx
Outdated
| {usersFromServer.map(user => { | ||
| return ( | ||
| <option key={user.id} value={user.id}> | ||
| {user.name} | ||
| </option> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
We can return right away instead of return statement if there's no logic happening inside the callback
| {usersFromServer.map(user => { | |
| return ( | |
| <option key={user.id} value={user.id}> | |
| {user.name} | |
| </option> | |
| ); | |
| })} | |
| {usersFromServer.map(user => ( | |
| <option key={user.id} value={user.id}> | |
| {user.name} | |
| </option> | |
| ) | |
| )} |
src/components/TodoList/TodoList.tsx
Outdated
| todos: TodoWithUser[]; | ||
| }; | ||
|
|
||
| export const TodoList = ({ todos }: Props) => { |
There was a problem hiding this comment.
I mentioned return-right-away approach somewhere above, this component is a great place to practice it, since we have 2 return statement that can be avoided here
DEMO LINK