reactMoviesListAddFormCopy1 #2680

SerhiiSemennikov · 2024-12-15T19:07:04Z

No description provided.

mateacademy-ai-mentor

Thank you for your hard work on this pull request! 😊 While you've made great progress, there are a few critical issues that need to be addressed to meet the task requirements:

Validation on Blur: The NewMovie component should validate fields on blur to ensure all required fields are entered, which is currently not fully implemented.
Disable Submit Button: The submit button should be disabled until all required fields are filled and trimmed, which is missing in the current implementation.
Error Handling: Errors should not persist after clearing the form. The form should be reinitialized, which is not currently happening.
getMaxId Function: This function is incorrectly implemented and may lead to errors in ID generation.
Props Handling: Ensure NewMovie receives all required props correctly, as errorMessage and titleErrorMessage are currently passed as empty strings.
Code Cleanup: Remove unnecessary commented-out code and redundant checks to improve clarity and maintainability.

Please address these issues to ensure the functionality aligns with the task requirements. Keep up the great work, and don't hesitate to reach out if you need further guidance! 🚀

✨ 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.

mateacademy-ai-mentor · 2024-12-15T19:08:11Z

src/App.tsx


 export const App = () => {
+  const visibleMovies = [...moviesFromServer];


You don't need to create a copy of the moviesFromServer array using the spread operator here. According to the checklist, if you are using a non-mutating array method, you don't need to create a copy of the array. You can directly use moviesFromServer.

mateacademy-ai-mentor · 2024-12-15T19:08:11Z

src/App.tsx

+        <NewMovie
+          onAdd={addMovie}
+          movies={movies}
+          errorMessage={''}
+          titleErrorMessage={''}


Ensure that the NewMovie component receives all required props correctly. Currently, errorMessage and titleErrorMessage are passed as empty strings. Verify if this is the intended behavior or if they should be dynamically set based on form validation results.

mateacademy-ai-mentor · 2024-12-15T19:08:11Z

src/components/MovieCard/MovieCard.tsx

+      <div className="media-content">
+        <p className="title is-8"></p>
+      </div>


The second media-content div is empty and does not serve any purpose. It seems to be a leftover from a copy-paste operation. Consider removing it to clean up the code.

mateacademy-ai-mentor · 2024-12-15T19:08:11Z

src/components/NewMovie/NewMovie.tsx

+function getMaxId(movies: Movie[]) {
+  return Math.max(...movies.map(m => movies.indexOf(m)));
+}


The getMaxId function is incorrectly implemented. It uses movies.indexOf(m) which will always return the index of the first occurrence of m, leading to incorrect ID generation. You should map to m.id or a similar property if Movie has an id field.

mateacademy-ai-mentor · 2024-12-15T19:08:11Z

src/components/NewMovie/NewMovie.tsx

+    //if (!title || !imdbUrl || !imgUrl) {
+    // return;
+    //}


This block of code is commented out but seems to be a necessary validation check. Consider uncommenting and using it to ensure that all required fields are filled before submission.

mateacademy-ai-mentor · 2024-12-15T19:08:12Z

src/components/NewMovie/NewMovie.tsx

+    if (!title || !imdbUrl || !imgUrl || title.length < 5) {
+      return;
+    }


This validation check is redundant because similar checks are already performed earlier. Consider removing this block to avoid unnecessary code.

mateacademy-ai-mentor · 2024-12-15T19:08:12Z

src/components/NewMovie/NewMovie.tsx

        required
+        errorMessage={titleErrorMessage}


Ensure that titleErrorMessage is correctly set based on the validation logic. Currently, it is only set when the title is missing or too short.

mateacademy-ai-mentor · 2024-12-15T19:08:12Z

src/components/NewMovie/NewMovie.tsx

+        value={imgUrl}
+        onChange={setImgUrl}
+        required
+        errorMessage={errorMessage}


The errorMessage is used for both imgUrl and imdbUrl fields. Consider using separate error messages for each field to provide more specific feedback to the user.

mateacademy-ai-mentor · 2024-12-15T19:08:12Z

src/components/TextField/TextField.tsx

@@ -49,8 +52,11 @@ export const TextField: React.FC<Props> = ({
          onBlur={() => setTouched(true)}
        />
      </div>
-
+      {errorMessage && (hasError = false)}


The line {errorMessage && (hasError = false)} is problematic because it directly modifies the hasError flag based on the presence of an errorMessage. This can lead to incorrect error handling logic. Consider restructuring the logic to ensure hasError is set correctly based on the intended conditions.

mateacademy-ai-mentor

Great effort on your submission! 🎉 While your code has been approved to help you move forward with your learning, there are several critical areas that need improvement:

Validation on Blur: The NewMovie component should validate fields like title, imgUrl, imdbUrl, and imdbId when they lose focus. Make sure to implement this to enhance user experience.
Disable Submit Button: Ensure the submit button is disabled until all required fields are filled and trimmed. This will prevent incomplete form submissions.
Error Handling: Errors should not persist after clearing the form. Reinitialize the form correctly to clear any lingering errors.
getMaxId Function: The current implementation is incorrect. It should return the maximum imdbId or another unique identifier, not the index of the movie. Consider using a property that uniquely identifies each movie.
Props Handling: The errorMessage and titleErrorMessage props are currently passed as empty strings. Ensure these are managed internally within the NewMovie component based on validation logic.
Code Cleanup: Remove any commented-out code and redundant checks to improve clarity and maintainability.

Addressing these issues will not only align your code with the task requirements but also enhance its functionality and maintainability. 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! ✨

mateacademy-ai-mentor · 2024-12-16T17:40:09Z

src/App.tsx

+          errorMessage={''}
+          titleErrorMessage={''}


The errorMessage and titleErrorMessage props are currently passed as empty strings. Ensure these props are correctly handled and passed meaningful values, as per the task requirements.

mateacademy-ai-mentor · 2024-12-16T17:40:10Z

src/components/NewMovie/NewMovie.tsx

+  titleErrorMessage: string;
+};
+function getMaxId(movies: Movie[]) {
+  return Math.max(...movies.map(movie => movies.indexOf(movie)));


The getMaxId function is incorrectly implemented. It should return the maximum imdbId or another unique identifier, not the index of the movie. Consider using a property of Movie that uniquely identifies it.

mateacademy-ai-mentor · 2024-12-16T17:40:10Z

src/components/NewMovie/NewMovie.tsx

+}
+
+export const NewMovie: React.FC<Props> = ({ onAdd, movies }) => {
+  const [count, setCount] = useState(getMaxId(movies) + 1);


The initial count is set using getMaxId, which is incorrectly implemented. Ensure count is initialized with a correct unique identifier.

mateacademy-ai-mentor · 2024-12-16T17:40:10Z

src/components/NewMovie/NewMovie.tsx

+    if (!title || !imdbUrl || !imgUrl || title.length < 5) {
+      return;
+    }


The submit button should be disabled until all required fields are filled and trimmed. Currently, this logic is not implemented, allowing form submission even when fields are incomplete.

mateacademy-ai-mentor · 2024-12-16T17:40:10Z

src/components/NewMovie/NewMovie.tsx

+  const [errorMessage, setErrorMessage] = useState('');
+  const [titleErrorMessage, setTitleErrorMessage] = useState('');


Ensure that error messages are cleared when the form is reset. This will prevent errors from persisting after clearing the form.

mateacademy-ai-mentor · 2024-12-16T17:40:10Z

src/components/TextField/TextField.tsx

@@ -49,8 +52,11 @@ export const TextField: React.FC<Props> = ({
          onBlur={() => setTouched(true)}
        />
      </div>
-
+      {errorMessage && (hasError = false)}


The logic here resets hasError when errorMessage is present. This can lead to unexpected behavior where the required field error is not shown even if the field is empty. Consider revising this logic to ensure both types of errors are handled correctly.

copy1

2118e10

mateacademy-ai-mentor suggested changes Dec 15, 2024

View reviewed changes

copy2

f879e45

SerhiiSemennikov requested a review from mateacademy-ai-mentor December 16, 2024 17:38

mateacademy-ai-mentor approved these changes Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reactMoviesListAddFormCopy1 #2680

reactMoviesListAddFormCopy1 #2680

SerhiiSemennikov commented Dec 15, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor Dec 15, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 16, 2024

mateacademy-ai-mentor Dec 16, 2024

mateacademy-ai-mentor Dec 16, 2024

mateacademy-ai-mentor Dec 16, 2024

mateacademy-ai-mentor Dec 16, 2024

mateacademy-ai-mentor Dec 16, 2024


		export const App = () => {
		const visibleMovies = [...moviesFromServer];

		const [errorMessage, setErrorMessage] = useState('');
		const [titleErrorMessage, setTitleErrorMessage] = useState('');

reactMoviesListAddFormCopy1 #2680

Are you sure you want to change the base?

reactMoviesListAddFormCopy1 #2680

Conversation

SerhiiSemennikov commented Dec 15, 2024

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment