-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
I need help #2748
base: master
Are you sure you want to change the base?
I need help #2748
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 areas for improvement, such as adding error handling in TodoModal.tsx
and ensuring type safety in TodoFilter.tsx
, these are minor issues that don't significantly impact the functionality. Keep up the good work and consider these suggestions for future enhancements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
export const App: React.FC = () => { | ||
const [todos, setTodos] = useState<Todo[]>([]); | ||
const [fullTodos, setFullTodos] = useState<Todo[]>([]); | ||
const [loading, setLoading] = useState(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.
The loading
state is set to true
initially and then set to false
in the finally
block of the fetchTodos
function. This could cause a flickering effect if the component re-renders unnecessarily. Consider checking if the todosList
is empty before setting loading
to false
to avoid unnecessary re-renders.
}; | ||
|
||
const handleStatusChange = (event: React.ChangeEvent<HTMLSelectElement>) => { | ||
setStatus(event.target.value); |
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.
Consider explicitly typing event.target.value
as string
in the handleStatusChange
function to ensure type safety. This can prevent potential runtime errors if the value is not of the expected type.
const fetchedUser = await getUser(todoInfo.userId); | ||
|
||
setUser(fetchedUser); | ||
} catch (error) { |
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.
Consider adding error handling logic inside the catch
block to handle potential errors when fetching user data. This could involve setting an error state or displaying an error message to the user.
DEMO LINK