Skip to content
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

reactMoviesListAddFormCopy1 #2680

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,4 @@ const pattern = /^((([A-Za-z]{3,9}:(?:\/\/)?)(?:[-;:&=+$,\w]+@)?[A-Za-z0-9.-]+|(
- Implement a solution following the [React task guideline](https://github.com/mate-academy/react_task-guideline#react-tasks-guideline).
- Use the [React TypeScript cheat sheet](https://mate-academy.github.io/fe-program/js/extra/react-typescript).
- Open one more terminal and run tests with `npm test` to ensure your solution is correct.
- Replace `<your_account>` with your Github username in the [DEMO LINK](https://<your_account>.github.io/react_movies-list-add-form/) and add it to the PR description.
- Replace `<your_account>` with your Github username in the [DEMO LINK](https://SerhiiSemennikov.github.io/react_movies-list-add-form/) and add it to the PR description.
20 changes: 18 additions & 2 deletions src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,31 @@ import './App.scss';
import { MoviesList } from './components/MoviesList';
import { NewMovie } from './components/NewMovie';
import moviesFromServer from './api/movies.json';
import React, { useState, useCallback } from 'react';
import { Movie } from './types/Movie';

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const [movies, setMovies] = useState<Movie[]>(visibleMovies);

const addMovie = useCallback((newMovie: Movie) => {
setMovies(currentMovies => {
return [...currentMovies, newMovie];
});
}, []);

return (
<div className="page">
<div className="page-content">
<MoviesList movies={moviesFromServer} />
<MoviesList movies={movies} />
</div>
<div className="sidebar">
<NewMovie /* onAdd={(movie) => {}} */ />
<NewMovie
onAdd={addMovie}
movies={movies}
errorMessage={''}
titleErrorMessage={''}
Comment on lines +23 to +27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +26 to +27

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/>
</div>
</div>
);
Expand Down
4 changes: 3 additions & 1 deletion src/components/MovieCard/MovieCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ export const MovieCard: React.FC<Props> = ({ movie }) => (
<p className="title is-8">{movie.title}</p>
</div>
</div>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<div className="content">
{movie.description}
<br />
Expand Down
114 changes: 105 additions & 9 deletions src/components/NewMovie/NewMovie.tsx
Original file line number Diff line number Diff line change
@@ -1,30 +1,126 @@
import { useState } from 'react';
import { TextField } from '../TextField';
import React from 'react';
import { Movie } from '../../types/Movie';

export const NewMovie = () => {
type Props = {
onAdd: (newMovie: Movie) => void;
//getMaxId: (movies: Movie[]) => number | (() => number);
movies: Movie[];
errorMessage: string;
titleErrorMessage: string;
};
function getMaxId(movies: Movie[]) {
return Math.max(...movies.map(m => movies.indexOf(m)));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


export const NewMovie: React.FC<Props> = ({ onAdd, movies }) => {
// Increase the count after successful form submission
// to reset touched status of all the `Field`s
const [count] = useState(0);

const [count, setCount] = useState(getMaxId(movies) + 1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const [title, setTitle] = useState('');
const [description, setDescription] = useState('');
const [imgUrl, setImgUrl] = useState('');
const [imdbUrl, setImdbUrl] = useState('');
const [imdbId, setImdbId] = useState('');
const [errorMessage, setErrorMessage] = useState('');
const [titleErrorMessage, setTitleErrorMessage] = useState('');
Comment on lines +25 to +26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const pattern =
// eslint-disable-next-line max-len
/^((([A-Za-z]{3,9}:(?:\/\/)?)(?:[-;:&=+$,\w]+@)?[A-Za-z0-9.-]+|(?:www\.|[-;:&=+$,\w]+@)[A-Za-z0-9.-]+)((?:\/[+~%/.\w-_]*)?\??(?:[-+=&;%@,.\w_]*)#?(?:[,.!/\\\w]*))?)$/;
const reset = () => {
setTitle('');
setImgUrl('');
setImdbUrl('');
setImdbId('');
setDescription('');
setTitleErrorMessage('');
setErrorMessage('');
};

const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();
const movie = {
title: title,
description: description,
imgUrl: imgUrl,
imdbUrl: imdbUrl,
imdbId: imdbId,
count: count,
};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!title) {
setTitleErrorMessage('should have some text');
} else if (title.length < 5) {
setTitleErrorMessage('should have at least 5 chars');
}

if (!pattern.test(imgUrl) || !pattern.test(imdbUrl)) {
setErrorMessage('not valid');

return;
}

if (!title || !imdbUrl || !imgUrl || title.length < 5) {
return;
}
Comment on lines +69 to +71

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +69 to +71

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


onAdd(movie);
setCount(currentCount => currentCount + 1);
reset();
};

return (
<form className="NewMovie" key={count}>
<form className="NewMovie" key={count} onSubmit={handleSubmit}>
<h2 className="title">Add a movie</h2>

<TextField
name="title"
label="Title"
value=""
onChange={() => {}}
value={title}
onChange={setTitle}
required
errorMessage={titleErrorMessage}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/>

<TextField name="description" label="Description" value="" />
<TextField
name="description"
label="Description"
value={description}
onChange={setDescription}
errorMessage=""
/>

<TextField name="imgUrl" label="Image URL" value="" />
<TextField
name="imgUrl"
label="Image URL"
value={imgUrl}
onChange={setImgUrl}
required
errorMessage={errorMessage}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/>

<TextField name="imdbUrl" label="Imdb URL" value="" />
<TextField
name="imdbUrl"
label="Imdb URL"
value={imdbUrl}
onChange={setImdbUrl}
required
errorMessage={errorMessage}
/>

<TextField name="imdbId" label="Imdb ID" value="" />
<TextField
name="imdbId"
label="Imdb ID"
value={imdbId}
onChange={setImdbId}
errorMessage=""
/>

<div className="field is-grouped">
<div className="control">
Expand Down
12 changes: 9 additions & 3 deletions src/components/TextField/TextField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ type Props = {
label?: string;
placeholder?: string;
required?: boolean;
errorMessage: string;

onChange?: (newValue: string) => void;
};

Expand All @@ -18,6 +20,7 @@ export const TextField: React.FC<Props> = ({
name,
value,
label = name,
errorMessage,
placeholder = `Enter ${label}`,
required = false,
onChange = () => {},
Expand All @@ -27,14 +30,14 @@ export const TextField: React.FC<Props> = ({

// To show errors only if the field was touched (onBlur)
const [touched, setTouched] = useState(false);
const hasError = touched && required && !value;

let hasError = touched && required && !value;

return (
<div className="field">
<label className="label" htmlFor={id}>
{label}
</label>

<div className="control">
<input
type="text"
Expand All @@ -49,8 +52,11 @@ export const TextField: React.FC<Props> = ({
onBlur={() => setTouched(true)}
/>
</div>

{errorMessage && (hasError = false)}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{hasError && <p className="help is-danger">{`${label} is required`}</p>}
{errorMessage && (
<p className="help is-danger">{`${label} ${errorMessage}`}</p>
)}
</div>
);
};
Loading