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

add task solution #2676

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

add task solution #2676

wants to merge 4 commits into from

Conversation

Mariagosp
Copy link

Copy link

@Andrii-Yanchuk Andrii-Yanchuk left a comment

Choose a reason for hiding this comment

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

Хороша робота👍. Але є моменти, які треба виправити. Під час завантаження модального вікна сторінка не показує користувачу, що йде загрузка, а просто все зникає і через секунду з'являється інформація


{' by '}

<a href="mailto:[email protected]">Leanne Graham</a>
<a href="mailto:[email protected]">{user.name}</a>

Choose a reason for hiding this comment

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

В ссилці на email можеш використовувати email з файла json

Andrii-Yanchuk

This comment was marked as duplicate.

Copy link

@Andrii-Yanchuk Andrii-Yanchuk left a comment

Choose a reason for hiding this comment

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

Хороша робота👍

Copy link

@VolodymyrKirichenko VolodymyrKirichenko left a comment

Choose a reason for hiding this comment

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

Looks good, but let's make some edits) 🥹

src/App.tsx Outdated
Comment on lines 16 to 17
const [query, setQuery] = useState<string>('');
const [status, setStatus] = useState<string>('all');

Choose a reason for hiding this comment

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

  1. If you pass the type as (string, number, boolean) - the compiler understands what type it is and you don't need to type it.
  2. u created a generic for this. Use it in ur state
Suggested change
const [query, setQuery] = useState<string>('');
const [status, setStatus] = useState<string>('all');
const [query, setQuery] = useState('');
const [status, setStatus] = useState<Filters>(Filters.ALL);

src/App.tsx Outdated
const [status, setStatus] = useState<string>('all');
const [selectedTodoId, setSelectedTodoId] = useState<number | null>(null);

const [loadingTodos, setLoadingTodos] = useState(true);

Choose a reason for hiding this comment

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

for boolean variables use prefixes (is , has )

Suggested change
const [loadingTodos, setLoadingTodos] = useState(true);
const [isLoadingTodos, setIsLoadingTodos] = useState(true);

src/App.tsx Outdated
Comment on lines 23 to 30
.filter(todo => todo.title.toLowerCase().includes(query.toLowerCase()))
.filter(todo => {
if (status === Filters.ALL) {
return true;
}

return status === Filters.COMPLETED ? todo.completed : !todo.completed;
});

Choose a reason for hiding this comment

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

instead use only 1 filter (logic in the middle of the filter method callback)

src/App.tsx Outdated
return status === Filters.COMPLETED ? todo.completed : !todo.completed;
});

const selected = todos.find(todo => todo.id === selectedTodoId);

Choose a reason for hiding this comment

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

selected what?change naming for this variable

src/App.tsx Outdated
Comment on lines 34 to 52
const onQueryChange = (event: React.ChangeEvent<HTMLInputElement>) => {
setQuery(event.target.value);
};

const onReset = () => {
setQuery('');
};

const onOpenModal = (todo: Todo) => {
setSelectedTodoId(todo.id);
};

const onCloseModal = () => {
setSelectedTodoId(null);
};

const onStatusChange = (event: React.ChangeEvent<HTMLSelectElement>) => {
setStatus(event.target.value as Filters);
};

Choose a reason for hiding this comment

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

use handle prefix for handle functions(in parent components) - naming-convention

Suggested change
const onQueryChange = (event: React.ChangeEvent<HTMLInputElement>) => {
setQuery(event.target.value);
};
const onReset = () => {
setQuery('');
};
const onOpenModal = (todo: Todo) => {
setSelectedTodoId(todo.id);
};
const onCloseModal = () => {
setSelectedTodoId(null);
};
const onStatusChange = (event: React.ChangeEvent<HTMLSelectElement>) => {
setStatus(event.target.value as Filters);
};
const handleQueryChange = (event: React.ChangeEvent<HTMLInputElement>) => {
setQuery(event.target.value);
};
const handleReset = () => {
setQuery('');
};
const handleOpenModal = (todo: Todo) => {
setSelectedTodoId(todo.id);
};
const handleCloseModal = () => {
setSelectedTodoId(null);
};
const handleStatusChange = (event: React.ChangeEvent<HTMLSelectElement>) => {
setStatus(event.target.value as Filters);
};

src/App.tsx Outdated
Comment on lines 81 to 88
{loadingTodos && todos.length === 0 && <Loader />}
{!loadingTodos && (
<TodoList
todos={filteredTodos}
selectedTodoId={selectedTodoId}
onOpenModal={onOpenModal}
/>
)}

Choose a reason for hiding this comment

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

Will be more readable and better

Suggested change
{loadingTodos && todos.length === 0 && <Loader />}
{!loadingTodos && (
<TodoList
todos={filteredTodos}
selectedTodoId={selectedTodoId}
onOpenModal={onOpenModal}
/>
)}
{(loadingTodos && !todos.length) ? (
<Loader />
) : (
<TodoList
todos={filteredTodos}
selectedTodoId={selectedTodoId}
onOpenModal={onOpenModal}
/>
)}

<i className="fas fa-magnifying-glass" />
</span>

<span className="icon is-right" style={{ pointerEvents: 'all' }}>

Choose a reason for hiding this comment

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

don't use inline styles

P.S. and yes, I know it was in the source code.
a programmer's job is to fix the bugs he sees)

<tr
data-cy="todo"
className={
selectedTodoId === todo.id ? 'has-background-info-light' : ''

Choose a reason for hiding this comment

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

use classnames library for it. Use this library for all such cases

export const TodoModal: React.FC<Props> = props => {
const { onCloseModal, todo } = props;

const [loading, setLoading] = useState(true);

Choose a reason for hiding this comment

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

the same issue(fix it for all such cases)

Suggested change
const [loading, setLoading] = useState(true);
const [isLoading, setIsLoading] = useState(true);

Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

Good work, approve it 👍
Small comments below

Comment on lines +19 to +21
<option value={Filters.ALL}>All</option>
<option value={Filters.ACTIVE}>Active</option>
<option value={Filters.COMPLETED}>Completed</option>

Choose a reason for hiding this comment

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

Would be good to use map here

setIsLoading(true);

getUser(todo.userId)
.then(data => setUser(data))

Choose a reason for hiding this comment

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

Suggested change
.then(data => setUser(data))
.then(setUser)

Comment on lines +2 to +4
ALL = 'all',
ACTIVE = 'active',
COMPLETED = 'completed',

Choose a reason for hiding this comment

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

Suggested change
ALL = 'all',
ACTIVE = 'active',
COMPLETED = 'completed',
All = 'all',
Active = 'active',
Completed = 'completed',

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.

4 participants