-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
task complete #1066
base: master
Are you sure you want to change the base?
task complete #1066
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,72 @@ | ||
import 'bulma/css/bulma.css'; | ||
import '@fortawesome/fontawesome-free/css/all.css'; | ||
import { Loader, TodoFilter, TodoList, TodoModal } from './components'; | ||
import { useEffect, useState } from 'react'; | ||
import { getTodos } from './api'; | ||
import { StatusType } from './types/Status'; | ||
import { useDispatch } from 'react-redux'; | ||
import { setTodos } from './features/todos'; | ||
import { setStatus } from './features/filter'; | ||
import { useAppSelector } from './app/hooks'; | ||
|
||
export const App = () => ( | ||
<> | ||
<div className="section"> | ||
<div className="container"> | ||
<div className="box"> | ||
<h1 className="title">Todos:</h1> | ||
export const App = () => { | ||
const dispatch = useDispatch(); | ||
const todos = useAppSelector(state => state.todos); | ||
const { query, status } = useAppSelector(state => state.filter); | ||
const selectTodo = useAppSelector(state => state.currentTodo); | ||
|
||
<div className="block"> | ||
<TodoFilter /> | ||
</div> | ||
const [isLoading, setIsLoading] = useState(false); | ||
|
||
useEffect(() => { | ||
setIsLoading(true); | ||
|
||
getTodos() | ||
.then(todosFromServer => { | ||
dispatch(setTodos(todosFromServer)); | ||
}) | ||
.catch(error => { | ||
throw new Error(error); | ||
}) | ||
.finally(() => { | ||
setIsLoading(false); | ||
dispatch(setStatus('all')); | ||
}); | ||
}, [dispatch]); | ||
Comment on lines
+20
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dependency array in useEffect: You've added |
||
|
||
const preparedTodos = todos | ||
.filter(todo => todo.title.toLowerCase().includes(query.toLowerCase())) | ||
.filter(todo => { | ||
switch (status) { | ||
case StatusType.Active: | ||
return !todo.completed; | ||
|
||
case StatusType.Completed: | ||
return todo.completed; | ||
|
||
default: | ||
return true; | ||
} | ||
}); | ||
Comment on lines
+36
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Performance issue: You're filtering your todos twice. This could be inefficient if your list of todos is very large. Consider combining these two filters into one to improve performance. |
||
|
||
return ( | ||
<> | ||
<div className="section"> | ||
<div className="container"> | ||
<div className="box"> | ||
<h1 className="title">Todos:</h1> | ||
|
||
<div className="block"> | ||
<TodoFilter /> | ||
</div> | ||
|
||
<div className="block"> | ||
<Loader /> | ||
<TodoList /> | ||
<div className="block"> | ||
{isLoading ? <Loader /> : <TodoList todos={preparedTodos} />} | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
|
||
<TodoModal /> | ||
</> | ||
); | ||
{selectTodo && <TodoModal />} | ||
</> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import { TypedUseSelectorHook, useSelector } from 'react-redux'; | ||
import { RootState } from './store'; | ||
|
||
export const useAppSelector: TypedUseSelectorHook<RootState> = useSelector; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,44 @@ | ||
import React from 'react'; | ||
import { Status, StatusType } from '../../types/Status'; | ||
import { useDispatch } from 'react-redux'; | ||
import { setQuery, setStatus } from '../../features/filter'; | ||
import { useAppSelector } from '../../app/hooks'; | ||
|
||
export const TodoFilter = () => { | ||
const dispatch = useDispatch(); | ||
const { status, query } = useAppSelector(state => state.filter); | ||
|
||
const showOptions = Object.values(StatusType).map(stat => ( | ||
<option key={stat} value={stat}> | ||
{stat[0].toUpperCase() + stat.slice(1)} | ||
</option> | ||
)); | ||
Comment on lines
+11
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
const handleChangeStatus = (event: React.ChangeEvent<HTMLSelectElement>) => { | ||
dispatch(setStatus(event.target.value as Status)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are casting the event target value to |
||
}; | ||
|
||
const handleChangeSearch = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
dispatch(setQuery(event.target.value)); | ||
}; | ||
|
||
const handleClearSearch = () => { | ||
dispatch(setQuery('')); | ||
}; | ||
|
||
export const TodoFilter: React.FC = () => { | ||
return ( | ||
<form | ||
className="field has-addons" | ||
onSubmit={event => event.preventDefault()} | ||
> | ||
<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 | ||
data-cy="statusSelect" | ||
value={status} | ||
onChange={handleChangeStatus} | ||
> | ||
{showOptions} | ||
</select> | ||
</span> | ||
</p> | ||
|
@@ -22,19 +49,24 @@ export const TodoFilter: React.FC = () => { | |
type="text" | ||
className="input" | ||
placeholder="Search..." | ||
value={query} | ||
onChange={handleChangeSearch} | ||
/> | ||
<span className="icon is-left"> | ||
<i className="fas fa-magnifying-glass" /> | ||
</span> | ||
|
||
<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> | ||
{!!query.length && ( | ||
<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" | ||
onClick={handleClearSearch} | ||
/> | ||
Comment on lines
+62
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While it's great that you're considering accessibility by using |
||
</span> | ||
)} | ||
</p> | ||
</form> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import cn from 'classnames'; | ||
import { Todo } from '../../types/Todo'; | ||
import { useAppSelector } from '../../app/hooks'; | ||
|
||
type Props = { | ||
todo: Todo; | ||
onSelect: (todo: Todo) => void; | ||
}; | ||
|
||
export const TodoItem: React.FC<Props> = ({ todo, onSelect }) => { | ||
const { id, title, completed } = todo; | ||
const currentTodo = useAppSelector(state => state.currentTodo); | ||
|
||
const handleSelectTodo = () => { | ||
onSelect(todo); | ||
}; | ||
|
||
return ( | ||
<tr data-cy="todo"> | ||
<td className="is-vcentered">{id}</td> | ||
<td className="is-vcentered"> | ||
{completed ? ( | ||
<span className="icon" data-cy="iconCompleted"> | ||
<i className="fas fa-check" /> | ||
</span> | ||
) : ( | ||
'' | ||
)} | ||
</td> | ||
|
||
<td className="is-vcentered is-expanded"> | ||
<p | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of the 'cn' library here seems unnecessary. You could directly write the ternary operator inside the className property. It would make the code cleaner and easier to read. |
||
className={cn({ | ||
'has-text-danger': !completed, | ||
'has-text-success': completed, | ||
})} | ||
> | ||
{title} | ||
</p> | ||
</td> | ||
|
||
<td className="has-text-right is-vcentered"> | ||
<button data-cy="selectButton" className="button" type="button"> | ||
{id === currentTodo?.id ? ( | ||
<span className="icon"> | ||
<i className="far fa-eye-slash" /> | ||
</span> | ||
) : ( | ||
<span className="icon" onClick={handleSelectTodo}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The onClick event is applied to the span element, it is better to apply it to the button element. This way, the entire button is clickable, not just the icon within the span. |
||
<i className="far fa-eye" /> | ||
</span> | ||
)} | ||
</button> | ||
</td> | ||
</tr> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './TodoItem'; |
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.
Error handling: You're throwing an error when the promise from
getTodos()
is rejected, but you're not catching it anywhere. This could potentially crash your application. Consider handling this error in a user-friendly way, for instance, by showing an error message to the user.