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

solved #2738

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

solved #2738

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
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 { useState } from 'react';

export type Movie = {
title: string;
description: string;
imgUrl: string;
imdbUrl: string;
imdbId: string;
};

export const App = () => {
const [movies, setMovie] = useState([...moviesFromServer]);

Choose a reason for hiding this comment

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

Consider renaming setMovie to setMovies to match the plural form of the movies state variable. This will improve code readability and maintain consistency in naming conventions.


const onAdd = (movie: Movie) => {
setMovie(currentMovies => [...currentMovies, movie]);
};

return (
<div className="page">
<div className="page-content">
<MoviesList movies={moviesFromServer} />
<MoviesList movies={movies} />
</div>
<div className="sidebar">
<NewMovie /* onAdd={(movie) => {}} */ />
<NewMovie onAdd={onAdd} />
</div>
</div>
);
Expand Down
74 changes: 62 additions & 12 deletions src/components/NewMovie/NewMovie.tsx
Original file line number Diff line number Diff line change
@@ -1,37 +1,87 @@
import { useState } from 'react';
import React, { useState } from 'react';
import { TextField } from '../TextField';
import { Movie } from '../../App';

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: (movie: Movie) => void;
};

export const NewMovie: React.FC<Props> = ({ onAdd }) => {
const [count, setCount] = useState(0);
const [title, setTitle] = useState('');
const [descr, setDescr] = useState('');
const [img, setImg] = useState('');
const [url, setUrl] = useState('');
const [id, setId] = useState('');
const isValid = title.trim() && img.trim() && url.trim() && id.trim();

const handleSubmit = (event: React.FormEvent) => {
event.preventDefault();

if (!isValid) {
return;
}

setCount(count + 1);
onAdd({
title,
description: descr,
imgUrl: img,
imdbUrl: url,
imdbId: id,
});
};

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

Choose a reason for hiding this comment

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

The onChange prop for TextField should be a function that takes an event and updates the state. Currently, setTitle is passed directly, which will not work as expected. Consider using an arrow function: onChange={(e) => setTitle(e.target.value)}.

/>

<TextField name="description" label="Description" value="" />
<TextField
name="description"
label="Description"
value={descr}
onChange={setDescr}
/>

Choose a reason for hiding this comment

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

Similar to the previous comment, the onChange handler for description should be updated to: onChange={(e) => setDescr(e.target.value)}.


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

Choose a reason for hiding this comment

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

The onChange handler for imgUrl should be updated to: onChange={(e) => setImg(e.target.value)}.

/>

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

Choose a reason for hiding this comment

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

The onChange handler for imdbUrl should be updated to: onChange={(e) => setUrl(e.target.value)}.

/>

<TextField name="imdbId" label="Imdb ID" value="" />
<TextField
name="imdbId"
label="Imdb ID"
value={id}
onChange={setId}
required

Choose a reason for hiding this comment

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

The onChange handler for imdbId should be updated to: onChange={(e) => setId(e.target.value)}.

/>

<div className="field is-grouped">
<div className="control">
<button
type="submit"
data-cy="submit-button"
className="button is-link"
disabled={!isValid}
>
Add
</button>
Expand Down
18 changes: 17 additions & 1 deletion src/components/TextField/TextField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
return Math.random().toFixed(16).slice(2);
}

const pattern =
/^((([A-Za-z]{3,9}:(?:\/\/)?)(?:[-;:&=+$,\w]+@)?[A-Za-z0-9.-]+|(?:www\.|[-;:&=+$,\w]+@)[A-Za-z0-9.-]+)((?:\/[+~%/.\w-_]*)?\??(?:[-+=&;%@,.\w_]*)#?(?:[,.!/\\\w]*))?)$/;

Check failure on line 18 in src/components/TextField/TextField.tsx

View workflow job for this annotation

GitHub Actions / run_linter (20.x)

This line has a length of 169. Maximum allowed is 80

Choose a reason for hiding this comment

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

The URL validation pattern seems overly complex and might not cover all valid URL cases. Consider simplifying the regex or using a library for URL validation to ensure comprehensive coverage.


export const TextField: React.FC<Props> = ({
name,
value,
Expand All @@ -28,6 +31,7 @@
// To show errors only if the field was touched (onBlur)
const [touched, setTouched] = useState(false);
const hasError = touched && required && !value;
const [validMessage, setValidMessage] = useState('');

return (
<div className="field">
Expand All @@ -46,11 +50,23 @@
placeholder={placeholder}
value={value}
onChange={event => onChange(event.target.value)}

Choose a reason for hiding this comment

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

The onChange handler correctly updates the parent component's state with the new input value. Ensure that the parent component's onChange prop is correctly implemented to handle this value.

onBlur={() => setTouched(true)}
onBlur={() => {
setTouched(true);
if (
(name === 'imgUrl' || name === 'imdbUrl') &&
!pattern.test(value)
) {
console.log(pattern.test(value));

Check failure on line 59 in src/components/TextField/TextField.tsx

View workflow job for this annotation

GitHub Actions / run_linter (20.x)

Unexpected console statement
setValidMessage('Not valid URL');
} else {
setValidMessage('');
}
}}
/>
</div>

{hasError && <p className="help is-danger">{`${label} is required`}</p>}
{validMessage && <p className="help is-danger">{`Not valid URL`}</p>}

Choose a reason for hiding this comment

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

The validation message 'Not valid URL' is hardcoded. Consider using the validMessage state variable to display the message, which allows for more flexibility and localization if needed.

</div>
);
};
Loading