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

solution #2090

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

solution #2090

wants to merge 6 commits into from

Conversation

pivkopa
Copy link

@pivkopa pivkopa commented Jan 15, 2024

Copy link

@kostrubitskii kostrubitskii left a comment

Choose a reason for hiding this comment

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

Good job , let's improve your code style!

const [imdbUrl, setImdbUrl] = useState('');
const [imdbId, setImdbId] = useState('');

// const [hasReset, setHasReset] = useState(false);

Choose a reason for hiding this comment

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

Delete unnecessery comments


// const [hasReset, setHasReset] = useState(false);

const able = titleValue.trim()

Choose a reason for hiding this comment

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

I think you need to rename your variable, for better understanding

Comment on lines 54 to 57
onSubmit={event => {
event.preventDefault();
submitAddMovie();
}}

Choose a reason for hiding this comment

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

Maybe you should move your "event.preventDefault();" to your function submitAddMovie.
And put here only link to your function

Suggested change
onSubmit={event => {
event.preventDefault();
submitAddMovie();
}}
onSubmit={submitAddMovie}

@pivkopa pivkopa requested a review from kostrubitskii January 16, 2024 09:35
Copy link

@olya-shyian olya-shyian left a comment

Choose a reason for hiding this comment

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

Great!🤗

<NewMovie /* onAdd={(movie) => {}} */ />
<NewMovie onAdd={(movie) => {
setPrepMovies(currentMovies => [...currentMovies, movie]);
}}

Choose a reason for hiding this comment

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

please create a separate handler for this

const [description, setDescription] = useState('');
const [imgUrlValue, 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.

instead of creating a lot of states, use just one

const defaultMovie = {
    title: '',
    description: '',
    imgUrl: '',
    imdbUrl: '',
    imdbId: '',
  };
  const [count, setCount] = useState(0);
  const [newMovie, setNewMovie] = useState(defaultMovie);

@pivkopa pivkopa requested a review from olya-shyian January 18, 2024 12:20
Copy link

@olya-shyian olya-shyian left a comment

Choose a reason for hiding this comment

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

Good work!✨

imdbUrl: newMovie.imdbUrl,
imdbId: newMovie.imdbId,
};
};

Choose a reason for hiding this comment

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

are you sure you need this function?

setCount((currentCount) => currentCount + 1);

onAdd(movie);
};

Choose a reason for hiding this comment

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

you can write more simple

const submitAddMovie  = (event: React.FormEvent<HTMLFormElement>) => {
    event.preventDefault();
    
    onAdd(newMovie);
    setNewMovie(defaultMovie);
    setCount((currentCount) => currentCount + 1);
  };

onSubmit={event => {
event.preventDefault();
submitAddMovie();
}}

Choose a reason for hiding this comment

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

and here write

 onSubmit={submitAddMovie}

@pivkopa pivkopa requested a review from olya-shyian January 18, 2024 16:25
Copy link

@YegorKorenkov YegorKorenkov left a comment

Choose a reason for hiding this comment

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

Please fix this mentor's comment from previous review/ You non need to create similar movie object which contain the same field like in newMovie
image

@pivkopa pivkopa requested a review from YegorKorenkov January 19, 2024 13:25
Copy link

@olya-shyian olya-shyian left a comment

Choose a reason for hiding this comment

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

Super! Thank you!✨

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