-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
didnt know tests failed #2565
base: master
Are you sure you want to change the base?
didnt know tests failed #2565
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.
Good job 👍
Let's make your code better
src/components/TodoList/TodoList.tsx
Outdated
<tr data-cy="todo" className="" key={todo.id}> | ||
<td className="is-vcentered">{todo.id}</td> | ||
{todo.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={ | ||
todo.completed ? 'has-text-success' : 'has-text-danger' | ||
} | ||
> | ||
{todo.title} | ||
</p> | ||
</td> | ||
<td className="has-text-right is-vcentered"> | ||
<button | ||
data-cy="selectButton" | ||
className="button" | ||
type="button" | ||
onClick={() => openModal(todo)} | ||
> | ||
<span className="icon"> | ||
<i |
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.
Move this logic to the TodoItem
component
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.
why? there are no tests for this component. As i understand i must using this "You are given the markup for the App, TodosList, TodoFilter, TodoModal and Loader components. Load data from the API and show it using the given components."
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.
After this fix, this component will be more readable, it will be a good practice
src/components/TodoList/TodoList.tsx
Outdated
<td className="is-vcentered is-expanded"> | ||
<p | ||
className={ | ||
todo.completed ? 'has-text-success' : 'has-text-danger' |
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.
Use the classnames library for add classes with condition, fix it everywhere
src/components/TodoList/TodoList.tsx
Outdated
todo.completed ? 'has-text-success' : 'has-text-danger' | ||
} | ||
> | ||
{todo.title} |
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.
Use destructuring for todo
|
||
export const App: React.FC = () => { | ||
const [todos, setTodos] = useState<Todo[]>([]); | ||
const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); |
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.
Redundant state
const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); |
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.
src/App.tsx
Outdated
getTodos().then(todos1 => { | ||
setTodos(todos1); | ||
setFilteredTodos(todos1); | ||
setLoading(false); | ||
}); |
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.
getTodos().then(todos1 => { | |
setTodos(todos1); | |
setFilteredTodos(todos1); | |
setLoading(false); | |
}); | |
getTodos() | |
.then(setTodos) | |
.finally(() => setLoading(false)); |
src/App.tsx
Outdated
|
||
export const App: React.FC = () => { | ||
const [todos, setTodos] = useState<Todo[]>([]); | ||
const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); | ||
const [loading, setLoading] = useState<boolean>(true); |
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.
const [loading, setLoading] = useState<boolean>(true); | |
const [isLoading, setIsLoading] = useState<boolean>(true); |
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.
Almost done!
You need to fix all comment for failed tests
|
||
export const App: React.FC = () => { | ||
const [todos, setTodos] = useState<Todo[]>([]); | ||
const [filteredTodos, setFilteredTodos] = useState<Todo[]>([]); |
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.
src/App.tsx
Outdated
const fetchTodos = async () => { | ||
const todosData = await getTodos(); | ||
|
||
setTodos(todosData); | ||
setFilteredTodos(todosData); | ||
}; | ||
|
||
fetchTodos(); |
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.
const fetchTodos = async () => { | |
const todosData = await getTodos(); | |
setTodos(todosData); | |
setFilteredTodos(todosData); | |
}; | |
fetchTodos(); | |
setIsLoading(true) | |
getTodos() | |
.then(setTodos) | |
.finally(() => setIsLoading(false)) |
src/App.tsx
Outdated
const [selectedTodo, setSelectedTodo] = useState<Todo | null>(null); | ||
|
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.
Create a state for isLoading
src/components/TodoList/TodoList.tsx
Outdated
<tr data-cy="todo" className="" key={todo.id}> | ||
<td className="is-vcentered">{todo.id}</td> | ||
{todo.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={ | ||
todo.completed ? 'has-text-success' : 'has-text-danger' | ||
} | ||
> | ||
{todo.title} | ||
</p> | ||
</td> | ||
<td className="has-text-right is-vcentered"> | ||
<button | ||
data-cy="selectButton" | ||
className="button" | ||
type="button" | ||
onClick={() => openModal(todo)} | ||
> | ||
<span className="icon"> | ||
<i |
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.
After this fix, this component will be more readable, it will be a good practice
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.
Great work!
DEMO LINK
Everything is works...but i cant pass the tests