-
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
some commit message #2746
base: master
Are you sure you want to change the base?
some commit message #2746
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 core functionality of the task! 🎉 While there are a few minor improvements suggested, such as ensuring imdbId
is not just whitespace and renaming setimdbId
for consistency, these do not significantly impact the overall 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 = () => { | ||
const [movies, setMovies] = useState(moviesFromServer); |
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 useState
hook is initialized with moviesFromServer
, which is imported from a JSON file. Ensure that moviesFromServer
is correctly formatted as an array of Movie
objects. If moviesFromServer
is not an array or contains invalid data, it might cause issues when rendering the MoviesList
component.
src/components/NewMovie/NewMovie.tsx
Outdated
const [description, setDescription] = useState(''); | ||
const [imgUrl, setImgUrl] = useState(''); | ||
const [imdbUrl, setImdbUrl] = useState(''); | ||
const [imdbId, setimdbId] = useState(''); |
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 renaming setimdbId
to setImdbId
for consistency with the camelCase naming convention used for other state setters.
src/components/NewMovie/NewMovie.tsx
Outdated
const [imdbUrl, setImdbUrl] = useState(''); | ||
const [imdbId, setimdbId] = useState(''); | ||
|
||
const isFormValid = title.trim() && imgUrl.trim() && imdbUrl.trim() && imdbId |
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 isFormValid
condition checks if imdbId
is truthy, but it does not ensure that imdbId
is a non-empty string. Consider using imdbId.trim()
to ensure it is not just whitespace.
|
||
<div className="field is-grouped"> | ||
<div className="control"> | ||
<button | ||
type="submit" | ||
data-cy="submit-button" | ||
className="button is-link" | ||
disabled={!isFormValid} |
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 disabled
attribute for the submit button is set based on isFormValid
. Ensure that isFormValid
correctly reflects the validity of all required fields, including proper URL validation for imgUrl
and imdbUrl
.
DEMO LINK