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

Solution #2682

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 51 additions & 5 deletions src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable max-len */
import React from 'react';
import React, { useEffect, useState } from 'react';
import 'bulma/css/bulma.css';
import '@fortawesome/fontawesome-free/css/all.css';

Expand All @@ -8,7 +8,39 @@ import { TodoFilter } from './components/TodoFilter';
import { TodoModal } from './components/TodoModal';
import { Loader } from './components/Loader';

import { Todo } from './types/Todo';
import { getTodos } from './api';
import { CompletedStatus } from './types/CompletedStatus';
export const App: React.FC = () => {
const [todos, setTodos] = useState<Todo[]>([]);
const [isLoading, setIsLoading] = useState<boolean>(false);
const [query, setQuery] = useState<string>('');
const [selectedTodoId, setSelectedTodoId] = useState<number | null>(null);

Choose a reason for hiding this comment

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

Якщо ми вказуємо конкретне значення в useState, то можно явно не прописувати його тип, він визначиться автоматично, наприклад,

const [query, setQuery] = useState('');

Він сам визначає, що це буде рядок.
Але якщо ми в useState прописуємо null, то тоді дійсно треба визначити, який тип там має бути

Choose a reason for hiding this comment

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

Хороша робота, вітаю тебе ))

Choose a reason for hiding this comment

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

Якщо ми вказуємо конкретне значення в useState, то можно явно не прописувати його тип, він визначиться автоматично, наприклад,

const [query, setQuery] = useState('');

Він сам визначає, що це буде рядок. Але якщо ми в useState прописуємо null, то тоді дійсно треба визначити, який тип там має бути

Good point

const [filterStatus, setFilterStatus] = useState<CompletedStatus>(
CompletedStatus.all,
);

let flag = null;

Choose a reason for hiding this comment

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

The variable flag is declared with let but is not reassigned. Consider using const instead for better code clarity and to prevent accidental reassignment.

Choose a reason for hiding this comment

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

Consider changing let flag = null; to const flag = null; for better code clarity and to prevent unintended reassignment.

Choose a reason for hiding this comment

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

What flag is it? I hope it's Ukrainian 🎏


if (filterStatus !== CompletedStatus.all) {
flag = filterStatus === CompletedStatus.completed ? true : false;
}
Comment on lines +25 to +27

Choose a reason for hiding this comment

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

This logic isn't obvious. It's better to create a function that incapsulates filter logic so you don't need to think about it in the component


const filteredTodos = todos.filter(todo => {
return (
todo.title.toLowerCase().includes(query.toLowerCase()) &&
(todo.completed === flag || flag === null)
);
});

useEffect(() => {
setIsLoading(true);
getTodos().then(response => {
setTodos(response);
setIsLoading(false);

Choose a reason for hiding this comment

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

You should remove the loading state in case of an error as well. In your case if an error occurs a user will see an infinite loader

});
}, []);

return (
<>
<div className="section">
Expand All @@ -17,18 +49,32 @@ export const App: React.FC = () => {
<h1 className="title">Todos:</h1>

<div className="block">
<TodoFilter />
<TodoFilter
query={query}
setQuery={setQuery}
setFilterStatus={setFilterStatus}
/>
</div>

<div className="block">
<Loader />
<TodoList />
{isLoading && <Loader />}
<TodoList
todos={filteredTodos}
selectedTodoId={selectedTodoId}
setSelectedTodoId={setSelectedTodoId}
/>
</div>
</div>
</div>
</div>

<TodoModal />
{selectedTodoId && (
<TodoModal
todos={todos}
selectedTodoId={selectedTodoId}
onClose={setSelectedTodoId}
/>
)}
</>
);
};
85 changes: 57 additions & 28 deletions src/components/TodoFilter/TodoFilter.tsx
Original file line number Diff line number Diff line change
@@ -1,30 +1,59 @@
export const TodoFilter = () => (
<form className="field has-addons">
<p className="control">
<span className="select">
<select data-cy="statusSelect">
<option value="all">All</option>
<option value="active">Active</option>
<option value="completed">Completed</option>
</select>
</span>
</p>
import { Dispatch, SetStateAction } from 'react';

<p className="control is-expanded has-icons-left has-icons-right">
<input
data-cy="searchInput"
type="text"
className="input"
placeholder="Search..."
/>
<span className="icon is-left">
<i className="fas fa-magnifying-glass" />
</span>
import { SetStateCompletedStatus } from '../../types/SetStateCompletedStatus';
import { CompletedStatus } from '../../types/CompletedStatus';

Choose a reason for hiding this comment

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

Ensure that the import SetStateCompletedStatus uses the correct Latin 'C' instead of a Cyrillic 'С'. This could lead to runtime errors if not corrected.


<span className="icon is-right" style={{ pointerEvents: 'all' }}>
{/* eslint-disable-next-line jsx-a11y/control-has-associated-label */}
<button data-cy="clearSearchButton" type="button" className="delete" />
</span>
</p>
</form>
);
export const TodoFilter: React.FC<{
query: string;
setQuery: Dispatch<SetStateAction<string>>;
setFilterStatus: SetStateCompletedStatus;
}> = ({ query, setQuery, setFilterStatus }) => {
function handleSearchChange(value: string) {
setQuery(value);
}

return (
<form className="field has-addons">
<p className="control">
<span className="select">
<select
onChange={event =>
setFilterStatus(event.target.value as CompletedStatus)
}
Comment on lines +20 to +21

Choose a reason for hiding this comment

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

Ensure that the value from event.target.value is correctly cast to the CompletedStatus type. This is crucial to prevent potential runtime errors.

data-cy="statusSelect"
>
<option value={CompletedStatus.all}>All</option>
<option value={CompletedStatus.active}>Active</option>
<option value={CompletedStatus.completed}>Completed</option>
</select>
</span>
</p>

<p className="control is-expanded has-icons-left has-icons-right">
<input
value={query}
onChange={event => handleSearchChange(event.target.value)}
data-cy="searchInput"
type="text"
className="input"
placeholder="Search..."
/>
<span className="icon is-left">
<i className="fas fa-magnifying-glass" />
</span>

{query && (
<span className="icon is-right " style={{ pointerEvents: 'all' }}>
{/* eslint-disable-next-line jsx-a11y/control-has-associated-label */}

Choose a reason for hiding this comment

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

Avoid inline styles, use classes instead

<button
onClick={() => handleSearchChange('')}
data-cy="clearSearchButton"
type="button"
className="delete"
/>
</span>
)}
</p>
</form>
);
};
169 changes: 75 additions & 94 deletions src/components/TodoList/TodoList.tsx
Original file line number Diff line number Diff line change
@@ -1,100 +1,81 @@
import { Todo } from '../../types/Todo';
import cN from 'classnames';
import React from 'react';
import { Dispatch, SetStateAction } from 'react';

export const TodoList: React.FC = () => (
<table className="table is-narrow is-fullwidth">
<thead>
<tr>
<th>#</th>
<th>
<span className="icon">
<i className="fas fa-check" />
</span>
</th>
<th>Title</th>
<th> </th>
</tr>
</thead>

<tbody>
<tr data-cy="todo" className="">
<td className="is-vcentered">1</td>
<td className="is-vcentered" />
<td className="is-vcentered is-expanded">
<p className="has-text-danger">delectus aut autem</p>
</td>
<td className="has-text-right is-vcentered">
<button data-cy="selectButton" className="button" type="button">
<span className="icon">
<i className="far fa-eye" />
</span>
</button>
</td>
</tr>
<tr data-cy="todo" className="has-background-info-light">
<td className="is-vcentered">2</td>
<td className="is-vcentered" />
<td className="is-vcentered is-expanded">
<p className="has-text-danger">quis ut nam facilis et officia qui</p>
</td>
<td className="has-text-right is-vcentered">
<button data-cy="selectButton" className="button" type="button">
<span className="icon">
<i className="far fa-eye-slash" />
</span>
</button>
</td>
</tr>

<tr data-cy="todo" className="">
<td className="is-vcentered">1</td>
<td className="is-vcentered" />
<td className="is-vcentered is-expanded">
<p className="has-text-danger">delectus aut autem</p>
</td>
<td className="has-text-right is-vcentered">
<button data-cy="selectButton" className="button" type="button">
export const TodoList: React.FC<{
todos: Todo[];
selectedTodoId: number | null;
setSelectedTodoId: Dispatch<SetStateAction<number | null>>;
}> = ({ todos, selectedTodoId, setSelectedTodoId }) => {
return (
<table className="table is-narrow is-fullwidth">
<thead>
<tr>
<th>#</th>
<th>
<span className="icon">
<i className="far fa-eye" />
<i className="fas fa-check" />
</span>
</button>
</td>
</tr>
</th>
<th>Title</th>
<th> </th>
</tr>
</thead>
<tbody>
{todos.map((todo: Todo) => {
const { id, title, completed } = todo;

<tr data-cy="todo" className="">
<td className="is-vcentered">6</td>
<td className="is-vcentered" />
<td className="is-vcentered is-expanded">
<p className="has-text-danger">
qui ullam ratione quibusdam voluptatem quia omnis
</p>
</td>
<td className="has-text-right is-vcentered">
<button data-cy="selectButton" className="button" type="button">
<span className="icon">
<i className="far fa-eye" />
</span>
</button>
</td>
</tr>
function onClickHandler() {

Choose a reason for hiding this comment

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

Suggested change
function onClickHandler() {
function hanldeClick() {

When you pass handler as a prop you should name it with on, when you use it inside the component it should start from handle. You don't need to combine prefixes in one name
https://javascript.plainenglish.io/handy-naming-conventions-for-event-handler-functions-props-in-react-fc1cbb791364

setSelectedTodoId(id);
}

<tr data-cy="todo" className="">
<td className="is-vcentered">8</td>
<td className="is-vcentered">
<span className="icon" data-cy="iconCompleted">
<i className="fas fa-check" />
</span>
</td>
<td className="is-vcentered is-expanded">
<p className="has-text-success">quo adipisci enim quam ut ab</p>
</td>
<td className="has-text-right is-vcentered">
<button data-cy="selectButton" className="button" type="button">
<span className="icon">
<i className="far fa-eye" />
</span>
</button>
</td>
</tr>
</tbody>
</table>
);
return (
<tr
key={id}
data-cy="todo"

Choose a reason for hiding this comment

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

Create a todo component

className={cN({
'has-background-info-light': selectedTodoId === id,
})}
>
<td className="is-vcentered">{id}</td>
{completed ? (
<td className="is-vcentered">
<span className="icon" data-cy="iconCompleted">
<i className="fas fa-check" />
</span>
</td>
) : (
<td className="is-vcentered" />
)}
<td className="is-vcentered is-expanded">
<p
className={completed ? 'has-text-success' : 'has-text-danger'}
>
{title}
</p>
</td>
<td className="has-text-right is-vcentered">
<button
onClick={onClickHandler}
data-cy="selectButton"
className="button"
type="button"
>
<span className="icon">
<i
className={cN(
'far',
selectedTodoId !== id ? 'fa-eye' : 'fa-eye-slash',

Choose a reason for hiding this comment

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

You compare selectedTodoId !== id twice, create a variable

)}
/>
</span>
</button>
</td>
</tr>
);
})}
</tbody>
</table>
);
};
Loading
Loading