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

movies list add form v1 #1889

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

Conversation

oltarasov
Copy link

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great start!
Screenshot 2023-10-11 at 13 26 12
Consider using trimming on error check and clearing the form after submit

src/App.tsx Outdated
imgUrl: string;
imdbUrl: string; imdbId: string; }) => {
setMovies(prevState => [...prevState, movie]);
}}

Choose a reason for hiding this comment

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

Consider moving type definitions outside JSX

});

// [setTitle, setDescription, setImgUrl, setImdbUrl, setImdbId]
// .forEach(func => func(''));

Choose a reason for hiding this comment

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

Remove unused code

Copy link

@loralevitska loralevitska left a comment

Choose a reason for hiding this comment

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

Almost perfect) Let's improve

  • using trimming after changing because I can't set spaces for title (that's the reason of fallen tests)
    image
    image

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

Choose a reason for hiding this comment

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

For what this spread operator, react created new array each render, and first value it's just default value which don't change moviesFromServer variable

Comment on lines 14 to 18
const [title, setTitle] = useState('');
const [description, setDescription] = useState('');
const [imgUrl, setImgUrl] = useState('');
const [imdbUrl, setImdbUrl] = useState('');
const [imdbId, setImdbId] = useState('');

Choose a reason for hiding this comment

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

Combine it into 1 state { title, description, ..... }

@@ -1,5 +1,6 @@
import classNames from 'classnames';
import React, { useState } from 'react';
// import { Movie } from '../../types/Movie';

Choose a reason for hiding this comment

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

Remove comment

Comment on lines 15 to 21
const [movieDef, setMovieDef] = useState({
title: '',
description: '',
imgUrl: '',
imdbUrl: '',
imdbId: '',
});

Choose a reason for hiding this comment

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

move this object to a variable (outside NewMovie component)

const initialMovieState = {
  title: '',
  description: '',
  imgUrl: '',
  imdbUrl: '',
  imdbId: '',
}
Suggested change
const [movieDef, setMovieDef] = useState({
title: '',
description: '',
imgUrl: '',
imdbUrl: '',
imdbId: '',
});
const [movieDef, setMovieDef] = useState(initialMovieState);

Comment on lines 37 to 43
setMovieDef({
title: '',
description: '',
imgUrl: '',
imdbUrl: '',
imdbId: '',
});

Choose a reason for hiding this comment

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

create a function reset and move this logic there. By the way, you can just pass initialMovieState

const handleSubmit = (event: React.FormEvent) => {
event.preventDefault();
setCount(prevState => prevState + 1);

Choose a reason for hiding this comment

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

before adding a movie you should validate values. Right now I can just press space for all form fields and submit a form

@oltarasov oltarasov requested a review from Viktor-Kost October 11, 2023 16:56
Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants