Skip to content
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

implementation #2160

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

implementation #2160

wants to merge 6 commits into from

Conversation

Daniilart01
Copy link

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

i can create the empty todo, let's fix this case firstly
image

import { User } from '../types/user';

export const findUserById = (id: number): User => (
usersFromServer.find(user => user.id === id) as User

Choose a reason for hiding this comment

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

find can return the undefined

Suggested change
usersFromServer.find(user => user.id === id) as User
usersFromServer.find(user => user.id === id) || null

<h2 className="TodoInfo__title">
{todo.title}
</h2>
<UserInfo user={findUserById(todo.userId)} />

Choose a reason for hiding this comment

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

user can be null, you need to handle this case here

Copy link
Author

@Daniilart01 Daniilart01 Nov 3, 2023

Choose a reason for hiding this comment

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

I decide to move this check using user?.name into TodoInfo component. In case User is null no info will be displayed, but in current implementation we have no possibilities to meet User === null, because when it's null we'll recive ann error 'Please choose user'

Choose a reason for hiding this comment

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

@Daniilart01 but you don't know where else this component can be reused so need to check if user exist

src/App.tsx Outdated
id="user"
data-cy="userSelect"
defaultValue={0}
onChange={event => handleUserChange(event)}

Choose a reason for hiding this comment

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

Suggested change
onChange={event => handleUserChange(event)}
onChange={handleUserChange}

src/App.tsx Outdated
Comment on lines 60 to 63
setFormData({
title: event.currentTarget.value,
user: formData.user,
});

Choose a reason for hiding this comment

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

Suggested change
setFormData({
title: event.currentTarget.value,
user: formData.user,
});
setFormData({
...formData,
title: event.currentTarget.value,
});

src/App.tsx Outdated
Comment on lines 12 to 13
const [emptyTitleFieldError, setEmptyTitleFieldError] = useState(false);
const [emptyUserFieldError, setEmptyUserFieldError] = useState(false);
Copy link

@andriy-shymkiv andriy-shymkiv Nov 4, 2023

Choose a reason for hiding this comment

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

I think enough just:

Suggested change
const [emptyTitleFieldError, setEmptyTitleFieldError] = useState(false);
const [emptyUserFieldError, setEmptyUserFieldError] = useState(false);
const [titleError, setTitleError] = useState(false);
const [userError, setUserError] = useState(false);

src/App.tsx Outdated
const [todosList, setTodosList] = useState(todosFromServer);

const [formData, setFormData]
= useState<{ title: string, user: User | null }>({ title: '', user: null });
Copy link

@andriy-shymkiv andriy-shymkiv Nov 4, 2023

Choose a reason for hiding this comment

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

better to create an interface for it:

interface FormFileds {
 title: string;
 user: User | null;
}

Choose a reason for hiding this comment

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

Suggested change
= useState<{ title: string, user: User | null }>({ title: '', user: null });
= useState<FormFields>({ title: '', user: null });

src/App.tsx Outdated
Comment on lines 29 to 34
setTodosList([...todosList, {
id: getNextTodoId(todosList),
title: formData.title.trim(),
completed: false,
userId: formData.user.id,
}]);

Choose a reason for hiding this comment

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

Suggested change
setTodosList([...todosList, {
id: getNextTodoId(todosList),
title: formData.title.trim(),
completed: false,
userId: formData.user.id,
}]);
setTodosList(prevTodos => [...prevTodos, {
id: getNextTodoId(todosList),
title: formData.title.trim(),
completed: false,
userId: formData.user.id,
}]);

<h2 className="TodoInfo__title">
{todo.title}
</h2>
<UserInfo user={findUserById(todo.userId)} />

Choose a reason for hiding this comment

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

@Daniilart01 but you don't know where else this component can be reused so need to check if user exist

Comment on lines 3 to 13
type Props = {
user: User | null
};

export const UserInfo: React.FC<Props> = ({ user }) => {
return (
<a className="UserInfo" href={`mailto:${user?.email}`}>
{user?.name}
</a>
);
};

Choose a reason for hiding this comment

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

Suggested change
type Props = {
user: User | null
};
export const UserInfo: React.FC<Props> = ({ user }) => {
return (
<a className="UserInfo" href={`mailto:${user?.email}`}>
{user?.name}
</a>
);
};
type Props = {
user: User;
};
export const UserInfo: React.FC<Props> = ({ user }) => {
return (
<a className="UserInfo" href={`mailto:${user.email}`}>
{user.name}
</a>
);
};

@Daniilart01
Copy link
Author

Daniilart01 commented Nov 4, 2023

now If userId === 0 it wouldn't be displayed on a todo. If we have undefined in formData.user this wont be proceed after add` button click

import { User } from '../types/user';

export const findUserById = (id: number): User => {
return usersFromServer.find(user => user.id === id) as User;

Choose a reason for hiding this comment

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

better to avoid the assertions in TS

Suggested change
return usersFromServer.find(user => user.id === id) as User;
return usersFromServer.find(user => user.id === id) || null;

import usersFromServer from '../api/users';
import { User } from '../types/user';

export const findUserById = (id: number): User => {

Choose a reason for hiding this comment

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

Suggested change
export const findUserById = (id: number): User => {
export const findUserById = (id: number): User | null => {

<h2 className="TodoInfo__title">
{todo.title}
</h2>
{todo.userId !== 0 && (<UserInfo user={findUserById(todo.userId)} />)}

Choose a reason for hiding this comment

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

move findUserById(todo.userId) this to some var and check if the user is not null

src/App.tsx Outdated
<input
placeholder="Enter a title"
id="title"
onInput={event => handleTitleChange(event)}

Choose a reason for hiding this comment

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

Suggested change
onInput={event => handleTitleChange(event)}
onInput={handleTitleChange}

we can simplify it

Choose a reason for hiding this comment

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

fix it everywhere

<h2 className="TodoInfo__title">
{todo.title}
</h2>
{todo.userId !== 0 && user && (<UserInfo user={user} />)}

Choose a reason for hiding this comment

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

Suggested change
{todo.userId !== 0 && user && (<UserInfo user={user} />)}
{user && (<UserInfo user={user} />)}

src/App.tsx Outdated
<input
placeholder="Enter a title"
id="title"
onInput={event => handleTitleChange(event)}

Choose a reason for hiding this comment

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

fix it everywhere

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

lgtm

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.

3 participants