react_movies-list-add-form #2677
Conversation
Yaroslav65
commented
Dec 14, 2024
Yaroslav65
commented
- DEMO LINK - https://Yaroslav65.github.io/react_movies-list-add-form/
There was a problem hiding this comment.
Thank you for your hard work on this Pull Request! 😊 While your implementation is mostly on point, there are a few critical issues that need to be addressed before we can proceed:
-
Duplicate Movies Issue: The
MoviesListcomponent is receiving bothmoviesFromServerandmovies, which can lead to duplicate entries. Ensure that onlymoviesis used, as it already includesmoviesFromServer. -
Form Validation: The
NewMovieform should utilize theonBlurevent for validation of required fields. Currently, this is not explicitly shown, so please ensure thatonBluris used to trigger validation. -
Submit Button Logic: The submit button should be disabled until all required fields are filled with trimmed values. Make sure trimming is applied before enabling the button.
-
Form Reset: After adding a new movie, the form should be cleared, and errors should not be shown. Double-check that the reset function also clears any error states.
Once these issues are resolved, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you have any questions. 🚀
✨ 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
| return ( | ||
| <div className="page"> | ||
| <div className="page-content"> | ||
| <MoviesList movies={moviesFromServer} /> | ||
| <MoviesList movies={[...moviesFromServer, ...movies]} /> |
There was a problem hiding this comment.
The MoviesList component is receiving both moviesFromServer and movies, which will result in duplicate entries if movies already contains movies from the server. Consider using only movies to avoid duplicates, as movies is initialized with moviesFromServer.
| const pattern = new RegExp( | ||
| '^((([A-Za-z]{3,9}:(?://)?)(?:[-;:&=+$,w]+@)?[A-Za-z0-9.-]+|(?:www.|' + | ||
| '[-;:&=+$,w]+@)[A-Za-z0-9.-]+)' + | ||
| '((?:\\/[+~%/\\.\\w-_]*)?\\??(?:[-+=&;%@,\\.\\w_]*)#?' + | ||
| '(?:[,.!/\\\\\\w]*))?)$', | ||
| ); |
There was a problem hiding this comment.
The regex pattern used for URL validation might not cover all valid URL formats. Consider using a more comprehensive regex or a library like 'validator' to ensure accurate URL validation.
There was a problem hiding this comment.
Great job on addressing most of the issues from the previous review! 🎉 Your implementation now effectively handles duplicate movies and ensures form validation is mostly in place. However, there are a few critical areas to focus on:
-
Duplicate Movies Issue: Ensure that
moviesFromServeris not separately passed toMoviesListto avoid duplicate entries. It's great that theMoviesListcomponent filters duplicates based onimdbId. -
Form Validation: Consider using the
onBlurevent for validation of fields likeimdbId,imdbUrl,imgUrl,description, andtitleto ensure they are filled correctly. This will enhance the user experience by providing immediate feedback. -
Submit Button Logic: Ensure the submit button is only enabled when all required fields are filled with trimmed values. This will prevent accidental submissions with incomplete data.
Your progress is commendable, and addressing these points will further strengthen your code. Keep up the great work and continue learning from these 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<Movie[]>(moviesFromServer); |
There was a problem hiding this comment.
The movies state is initialized with moviesFromServer, which is correct. However, ensure that moviesFromServer is not separately passed to MoviesList to avoid duplicate entries.
| return ( | ||
| <div className="page"> | ||
| <div className="page-content"> | ||
| <MoviesList movies={moviesFromServer} /> | ||
| <MoviesList movies={[...movies]} /> |
There was a problem hiding this comment.
Ensure that moviesFromServer is not separately passed to MoviesList to avoid duplicate entries. The spread operator here is correct if movies is the only source of truth for the movie list.
| value="" | ||
| onChange={() => {}} | ||
| value={title} | ||
| onChange={setTitle} |
There was a problem hiding this comment.
Consider using the onBlur event for validation of the title field to ensure it is filled correctly.
| name="description" | ||
| label="Description" | ||
| value={description} | ||
| onChange={setDescription} |
There was a problem hiding this comment.
Consider using the onBlur event for validation of the description field if it is required.
| onChange={value => { | ||
| setImgUrl(value); | ||
| setImgUrlError(null); | ||
| }} |
There was a problem hiding this comment.
Consider using the onBlur event for validation of the imgUrl field to ensure it is filled correctly.
| onChange={value => { | ||
| setImdbUrl(value); | ||
| setImdbUrlError(null); | ||
| }} |
There was a problem hiding this comment.
Consider using the onBlur event for validation of the imdbUrl field to ensure it is filled correctly.
| name="imdbId" | ||
| label="Imdb ID" | ||
| value={imdbId} | ||
| onChange={setImdbId} |
There was a problem hiding this comment.
Consider using the onBlur event for validation of the imdbId field to ensure it is filled correctly.
|
|
||
| <div className="field is-grouped"> | ||
| <div className="control"> | ||
| <button | ||
| type="submit" | ||
| data-cy="submit-button" | ||
| className="button is-link" | ||
| disabled={!title || !imgUrl || !imdbUrl || !imdbId} |
There was a problem hiding this comment.
Ensure that the submit button is only enabled when all required fields are filled with trimmed values. Currently, it checks for non-empty values, but trimming should be applied.
