Develop #2713
Develop #2713
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 While your code is mostly on the right track, there are a few critical issues that need to be addressed before we can approve it. These include typographical errors in state variable names, which can lead to confusion and bugs, and the need for proper validation and error handling in the NewMovie component. Additionally, ensure that the getImageById function's behavior aligns with the application's requirements. Once these issues are resolved, your code will be in great shape! Keep up the good 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.
| function getImageById(imdbID: string): string | null { | ||
| return ( | ||
| moviesFromServer.find(movie => movie.imdbId === imdbID)?.imgUrl || null | ||
| ); | ||
| } |
There was a problem hiding this comment.
The getImageById function returns null if no movie with the given imdbId is found. Ensure that this behavior is intended and handled appropriately in the application. If an image is required for every movie, you might want to handle the null case differently.
src/components/NewMovie/NewMovie.tsx
Outdated
| const [hasTitleError, setHasTitleError] = useState(false); | ||
|
|
||
| const [description, setDescription] = useState(''); | ||
| const [hasdesctiptionError, setHasDesctiptionError] = useState(false); |
There was a problem hiding this comment.
There is a typo in the state variable name hasdesctiptionError. It should be hasDescriptionError to maintain consistency and avoid confusion.
src/components/NewMovie/NewMovie.tsx
Outdated
| const [imgUrl, setImgUrl] = useState(''); | ||
| const [hasImageUrlError, setHasImageUrlError] = useState(false); | ||
|
|
||
| const [imdbUrl, setImdbdUrl] = useState(''); |
There was a problem hiding this comment.
There is a typo in the state variable name setImdbdUrl. It should be setImdbUrl to match the naming convention used elsewhere in the code.
src/components/NewMovie/NewMovie.tsx
Outdated
| // to reset touched status of all the `Field`s | ||
| const [count] = useState(0); | ||
| setHasTitleError(!title); | ||
| // setHasDesctiptionError(!description); |
There was a problem hiding this comment.
The line for setting the description error state is commented out. If you intend to validate the description field, you should uncomment this line to ensure that the error state is set correctly.
src/components/NewMovie/NewMovie.tsx
Outdated
| value={description} | ||
| onChange={handleDesctiptionChange} | ||
| required | ||
| error={hasdesctiptionError} |
There was a problem hiding this comment.
The error prop for the description field uses hasdesctiptionError, which contains a typo. It should be hasDescriptionError to match the corrected state variable name.
| onChange = () => {}, | ||
| }) => { | ||
| // generate a unique id once on component load | ||
| const [id] = useState(() => `${name}-${getRandomDigits()}`); | ||
|
|
||
| // To show errors only if the field was touched (onBlur) | ||
| const [touched, setTouched] = useState(false); | ||
| const hasError = touched && required && !value; | ||
| const hasError = error || (touched && required && !value); |
There was a problem hiding this comment.
The hasError logic checks if the field is required and not filled, but it only triggers after the field is touched. This is correct, but ensure that this behavior aligns with the overall form validation strategy. If you want immediate validation feedback, consider adjusting this logic.
There was a problem hiding this comment.
Thank you for your submission! 🎉 Your code is on the right track, but there are a few critical issues that need attention to ensure robustness and alignment with the task requirements:
-
Typographical Errors: There are typographical errors in the function and state variable names, such as
handleDesctiptionChangeandsetHasDesctiptionError. These should be corrected tohandleDescriptionChangeandsetHasDescriptionErrorrespectively, to maintain consistency and avoid confusion. -
Validation and Error Handling: In the
NewMoviecomponent, ensure that all required fields are validated correctly before submission. Thedescriptionfield is optional, so adjust the validation logic accordingly. -
getImageByIdFunction: The function currently returnsnullif no image is found. Make sure this behavior aligns with the application's requirements. Consider handling this case appropriately in theMoviesListcomponent or when setting the movie data to avoid rendering issues.
Please address these issues to improve your code. This decision to approve is made to help you move forward with your learning. Remember to review the comments and make necessary improvements. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| function getImageById(imdbID: string): string | null { | ||
| return ( | ||
| moviesFromServer.find(movie => movie.imdbId === imdbID)?.imgUrl || null | ||
| ); | ||
| } |
There was a problem hiding this comment.
The getImageById function currently returns null if no image is found. Ensure this behavior aligns with the application's requirements. According to the checklist, you should not render components if the property passed to them is null or undefined. Consider handling this case appropriately in the MoviesList component or when setting the movie data.
| const [hasTitleError, setHasTitleError] = useState(false); | ||
|
|
||
| const [description, setDescription] = useState(''); | ||
| const [hasDescriptionError, setHasDesctiptionError] = useState(false); |
There was a problem hiding this comment.
There is a typographical error in the state variable name setHasDesctiptionError. It should be setHasDescriptionError to match the corresponding state variable hasDescriptionError.
| setHasTitleError(false); | ||
| }; | ||
|
|
||
| const handleDesctiptionChange = (value: string) => { |
There was a problem hiding this comment.
There is a typographical error in the function name handleDesctiptionChange. It should be handleDescriptionChange to match the corresponding state variable description.
https://taniavozniuk.github.io/react_movies-list-add-form/