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 all commits
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.
19 changes: 17 additions & 2 deletions src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,30 @@ 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 [movies, setMovies] = useState<Movie[]>(moviesFromServer);

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
118 changes: 107 additions & 11 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 = () => {
// Increase the count after successful form submission
// to reset touched status of all the `Field`s
const [count] = useState(0);
type Props = {
onAdd: (newMovie: Movie) => void;

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

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

}

export const NewMovie: React.FC<Props> = ({ onAdd, movies }) => {
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 (movies.find(el => el.title === title)?.title) {
setTitleErrorMessage('this title already exist');

return;
}

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