Develop #2749
Develop #2749
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed to meet the core requirements of the task:
-
Form Submission Handling: The
NewMoviecomponent should use the form'sonSubmitevent instead of the button'sonClickevent. This ensures proper form submission behavior, including handling the Enter key. -
Form Clearing and Error Handling: After adding a new movie, the form should be cleared, and errors should not persist. This can be achieved by reinitializing the form.
-
State Naming: The state variable managing the list of movies should be renamed from
posttomoviesfor better clarity and readability.
Please address these issues to ensure the functionality aligns with the task requirements. Keep up the great work, and don't hesitate to reach out if you need any help! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/App.tsx
Outdated
|
|
||
| export const App = () => { | ||
| const [post, setPost] = useState(moviesFromServer); |
There was a problem hiding this comment.
The state variable post and its setter setPost are used to manage a list of movies. It would be more intuitive to name these variables movies and setMovies to better reflect their purpose and improve code readability.
src/components/NewMovie/NewMovie.tsx
Outdated
| imdbUrl.trim() !== '' && | ||
| imdbId.trim() !== ''; | ||
|
|
||
| const onSubmit = (event: React.FormEvent<HTMLButtonElement>) => { |
There was a problem hiding this comment.
The onSubmit function is attached to the button's onClick event. It would be more appropriate to attach it to the form's onSubmit event to ensure that the form submission is handled correctly, even if the user submits the form using the Enter key. This change would also allow you to remove the type="submit" attribute from the button.
| @@ -27,7 +28,15 @@ export const TextField: React.FC<Props> = ({ | |||
|
|
|||
| // To show errors only if the field was touched (onBlur) | |||
| const [touched, setTouched] = useState(false); | |||
| const hasError = touched && required && !value; | |||
| const [generatedValue, setGeneratedValue] = useState(''); | |||
There was a problem hiding this comment.
The generatedValue state is used to manage the input value, but the value prop is also being passed to the input field. This can lead to inconsistencies if the value prop is controlled externally. Consider using only the value prop for the input's value and removing the generatedValue state to avoid conflicts.
There was a problem hiding this comment.
Review Summary
Great job on implementing the main functionality of managing and displaying a list of movies! 🎬 The App.tsx file is mostly well-implemented, but there's a critical issue with the naming of the addPost function. It should be renamed to addMovie to better reflect its purpose. This aligns with the previous suggestion to rename the state variable from post to movies for clarity.
The NewMovie component is well-structured, handling form submission correctly and meeting task requirements effectively. Similarly, the TextField component is well-implemented, providing a reusable input field with validation and error handling.
You've addressed previous issues, such as using the form's onSubmit event, clearing the form after submission, and improving state naming. However, ensure that the parameter name currentPost is updated to currentMovies or similar for consistency.
The decision to approve is made to help you move forward with your learning. Please review the comments to improve your code. Addressing these issues will enhance the clarity and functionality of your code in future projects. Keep up the good work and continue learning from your experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| export const App = () => { | ||
| const [movies, setMovies] = useState(moviesFromServer); | ||
|
|
||
| const addPost = (newPost: Movie) => { |
There was a problem hiding this comment.
The function addPost should be renamed to addMovie to better reflect its purpose of adding a movie to the list. This aligns with the previous suggestion to rename the state variable from post to movies for clarity.
| const [movies, setMovies] = useState(moviesFromServer); | ||
|
|
||
| const addPost = (newPost: Movie) => { | ||
| setMovies(currentPost => [...currentPost, newPost]); |
There was a problem hiding this comment.
Ensure that the parameter name currentPost is updated to currentMovies or similar, to maintain consistency with the renaming of the state variable and function.
No description provided.