solved #2738
solved #2738
Conversation
There was a problem hiding this comment.
Great job on implementing the core functionality and meeting the task requirements! 🎉 While there are a few minor improvements suggested, such as refining the URL validation and updating some onChange handlers for better consistency, these do not impact the overall functionality of your code. 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! ✨
src/App.tsx
Outdated
|
|
||
| export const App = () => { | ||
| const [movies, setMovie] = useState([...moviesFromServer]); |
There was a problem hiding this comment.
Consider renaming setMovie to setMovies to match the plural form of the movies state variable. This will improve code readability and maintain consistency in naming conventions.
src/components/NewMovie/NewMovie.tsx
Outdated
| value="" | ||
| onChange={() => {}} | ||
| value={title} | ||
| onChange={setTitle} |
There was a problem hiding this comment.
The onChange prop for TextField should be a function that takes an event and updates the state. Currently, setTitle is passed directly, which will not work as expected. Consider using an arrow function: onChange={(e) => setTitle(e.target.value)}.
src/components/NewMovie/NewMovie.tsx
Outdated
| name="description" | ||
| label="Description" | ||
| value={descr} | ||
| onChange={setDescr} |
There was a problem hiding this comment.
Similar to the previous comment, the onChange handler for description should be updated to: onChange={(e) => setDescr(e.target.value)}.
src/components/NewMovie/NewMovie.tsx
Outdated
| name="imgUrl" | ||
| label="Image URL" | ||
| value={img} | ||
| onChange={setImg} |
There was a problem hiding this comment.
The onChange handler for imgUrl should be updated to: onChange={(e) => setImg(e.target.value)}.
src/components/NewMovie/NewMovie.tsx
Outdated
| name="imdbUrl" | ||
| label="Imdb URL" | ||
| value={url} | ||
| onChange={setUrl} |
There was a problem hiding this comment.
The onChange handler for imdbUrl should be updated to: onChange={(e) => setUrl(e.target.value)}.
src/components/NewMovie/NewMovie.tsx
Outdated
| name="imdbId" | ||
| label="Imdb ID" | ||
| value={id} | ||
| onChange={setId} |
There was a problem hiding this comment.
The onChange handler for imdbId should be updated to: onChange={(e) => setId(e.target.value)}.
| @@ -14,6 +14,9 @@ | |||
| return Math.random().toFixed(16).slice(2); | |||
| } | |||
|
|
|||
| const pattern = | |||
| /^((([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 URL validation pattern seems overly complex and might not cover all valid URL cases. Consider simplifying the regex or using a library for URL validation to ensure comprehensive coverage.
| @@ -46,11 +50,23 @@ | |||
| placeholder={placeholder} | |||
| value={value} | |||
| onChange={event => onChange(event.target.value)} | |||
There was a problem hiding this comment.
The onChange handler correctly updates the parent component's state with the new input value. Ensure that the parent component's onChange prop is correctly implemented to handle this value.
| /> | ||
| </div> | ||
|
|
||
| {hasError && <p className="help is-danger">{`${label} is required`}</p>} | ||
| {validMessage && <p className="help is-danger">{`Not valid URL`}</p>} |
There was a problem hiding this comment.
The validation message 'Not valid URL' is hardcoded. Consider using the validMessage state variable to display the message, which allows for more flexibility and localization if needed.
Solved