-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Create logic of new film form #2681
base: master
Are you sure you want to change the base?
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.
Hello! 😊 Thank you for your hard work on this project. There are a few critical issues that need to be addressed before we can approve the pull request:
-
Validation on Blur: The form should validate the
title
,imgUrl
,imdbUrl
, andimdbId
fields when they lose focus (onBlur
). This is currently missing, and it's important for ensuring data integrity. -
Error Handling: Errors should not persist after the form is cleared. Implement a mechanism to reset errors when the form is reinitialized.
-
Use of
onBlur
: Ensure that theTextField
components utilize theonBlur
event for validation, rather than justonChange
. -
Regex Validation for URLs: Implement the optional advanced validation for
imgUrl
andimdbUrl
using the provided regex pattern to ensure they are valid URLs. -
Code Style: Avoid using one-letter variable names. Use descriptive names to improve code readability and maintainability.
-
Form Reinitialization: Change the form's key to ensure errors are not shown after clearing the form.
Once these issues are addressed, your code will be in great shape! Keep up the great work, and I'm looking forward to seeing the improvements. 🚀
✨ 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/components/NewMovie/NewMovie.tsx
Outdated
<TextField name="imdbUrl" label="Imdb URL" value="" /> | ||
<TextField | ||
name="imdbUrl" | ||
label="Imdb URsL" |
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.
There is a typo in the label 'Imdb URsL'. It should be 'Imdb URL'.
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 work on implementing the main functionality of the App.tsx
, NewMovie.tsx
, and TextField.tsx
components! 🎉 Your code is well-structured, and you've addressed most of the issues from previous reviews. Here are a few points to consider for further improvement:
-
Validation on Blur: Ensure that all fields, including
imdbId
,imdbUrl
,imgUrl
, anddescription
, are validated on blur using theonBlur
event. This will help in validating fields when they lose focus, which is crucial for user experience. -
Error Handling: You've done a good job with resetting errors by updating the form's key. Make sure this mechanism is consistently applied to prevent errors from persisting after the form is cleared.
-
Critical Issues: While there are no critical issues that block functionality, it's important to address the validation on blur for all fields to meet the task requirements fully.
Keep up the excellent work, and continue to refine your code by addressing these points. This decision is made to help you move forward with your learning. Remember to review the comments and improve your code to avoid similar issues in the future. Keep learning and growing! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
onChange={inputValue => { | ||
setNewMovie({ | ||
title: inputValue, | ||
description: newMovie.description, | ||
imgUrl: newMovie.imgUrl, | ||
imdbUrl: newMovie.imdbUrl, | ||
imdbId: newMovie.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 TextField
component should utilize the onBlur
event for validation, not just onChange
. This is important for ensuring that fields are validated when they lose focus, as per the task requirements.
onChange={inputValue => { | ||
setNewMovie({ | ||
title: newMovie.title, | ||
description: inputValue, | ||
imgUrl: newMovie.imgUrl, | ||
imdbUrl: newMovie.imdbUrl, | ||
imdbId: newMovie.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.
Ensure that the TextField
component for description
also uses the onBlur
event for validation, in addition to onChange
.
onChange={inputValue => { | ||
setNewMovie({ | ||
title: newMovie.title, | ||
description: newMovie.description, | ||
imgUrl: inputValue, | ||
imdbUrl: newMovie.imdbUrl, | ||
imdbId: newMovie.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 imgUrl
field should also be validated on blur using the onBlur
event. This is necessary to ensure that the URL is validated when the field loses focus.
onChange={inputValue => { | ||
setNewMovie({ | ||
title: newMovie.title, | ||
description: newMovie.description, | ||
imgUrl: newMovie.imgUrl, | ||
imdbUrl: inputValue, | ||
imdbId: newMovie.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 imdbUrl
field should be validated on blur using the onBlur
event to ensure the URL is valid when the field loses focus.
onChange={inputValue => { | ||
setNewMovie({ | ||
title: newMovie.title, | ||
description: newMovie.description, | ||
imgUrl: newMovie.imgUrl, | ||
imdbUrl: newMovie.imdbUrl, | ||
imdbId: inputValue, | ||
}); | ||
}} |
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 imdbId
field should also be validated on blur using the onBlur
event to ensure the field is validated when it loses focus.
|
||
const handleSubmit = (event: React.FormEvent) => { | ||
event.preventDefault(); | ||
onAdd(newMovie); |
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 implementing a mechanism to reset errors when the form is reinitialized. This will prevent errors from persisting after the form is cleared.
No description provided.