-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Develop #2733
base: master
Are you sure you want to change the base?
Develop #2733
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.
Great job on implementing the task! 🎉 While there are a few minor issues, such as a typo in a variable name and a hardcoded email address, they don't significantly impact the functionality. Consider addressing these for improved clarity and consistency. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/App.tsx
Outdated
filter: string, | ||
): Todo[] { | ||
const filtered = filter === 'all'; | ||
const convertedFiler = filter === 'completed' ? true : 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.
There is a typo in the variable name convertedFiler
. It should be convertedFilter
.
src/App.tsx
Outdated
const filtered = filter === 'all'; | ||
const convertedFiler = filter === 'completed' ? true : false; | ||
|
||
return todos.filter( | ||
todo => | ||
todo.title.toLowerCase().includes(cleanQuery) && | ||
(filtered || convertedFiler === todo.completed), |
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.
The logic for filtering todos based on their completion status might not be intuitive. Consider revising the logic to make it clearer. For example, you could directly use the filter value to determine the filtering condition.
</form> | ||
); | ||
query: string; | ||
onQueryChange: (Value: string) => void; |
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.
The parameter Value
should be value
to maintain consistent casing in the Props
type definition.
src/components/TodoList/TodoList.tsx
Outdated
type Props = { | ||
todos: Todo[]; | ||
selectedTodoId?: number | null; | ||
onSelect?: (userId: Todo) => void; |
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.
The parameter name userId
in the onSelect
function type should be todo
to accurately reflect the data being passed.
|
||
<a href="mailto:[email protected]">Leanne Graham</a> | ||
</p> | ||
<a href="mailto:[email protected]">{user.name}</a> |
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.
The email address in the href
attribute is hardcoded. It should be dynamically set using user.email
to ensure the correct email is displayed for each user.
DEMO LINK