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

branch in development (uploading to check tests) #1920

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

Conversation

Drach-Illya
Copy link

@Drach-Illya Drach-Illya commented Oct 21, 2023

DEMO LINK

Please be aware that this is an unfinished version. As the command 'npm test' always times out, I'm making this pull request to check tests and see what I need to improve. However, feel free to add feedback if you wish.

Update: Tests were successful!

Copy link

@StasSokolov1 StasSokolov1 left a comment

Choose a reason for hiding this comment

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

Great job, let's improve your code a bit more!

Comment on lines +30 to +32
<NewMovie onAdd={(movie) => {
addMovie(movie);
}}

Choose a reason for hiding this comment

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

Suggested change
<NewMovie onAdd={(movie) => {
addMovie(movie);
}}
<NewMovie onAdd={addMovie}

can be simplify

Copy link
Author

Choose a reason for hiding this comment

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

I don't think this is valid as we need to Recieve a movie value from a prop.
Writing it your way will make it hard to recieve the Object with info about movie from the component NewMovie

image

Choose a reason for hiding this comment

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

ok, let's leave it!

};

const disabledCondition
= titleValue === ''

Choose a reason for hiding this comment

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

Suggested change
= titleValue === ''
= !titleValue

can be simplify, check please below also

};

const reset = () => {
setCount(count + 1);

Choose a reason for hiding this comment

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

is this correct? when user click reset button the count is increase? i think should be initial value

Copy link
Author

Choose a reason for hiding this comment

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

// Increase the count after successful form submission
// to reset touched status of all the Fields

Yes, the comment in Master branch requests this to for reset

Copy link
Author

@Drach-Illya Drach-Illya Oct 23, 2023

Choose a reason for hiding this comment

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

Also, there is no reset button. Reset is done on the press of submit to clear the fields, which also ticks up count to prevent error message from appearing.

Comment on lines +52 to +53
onAdd(movie);
reset();

Choose a reason for hiding this comment

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

in this place when user click add button the count should be increase

Copy link
Author

@Drach-Illya Drach-Illya Oct 23, 2023

Choose a reason for hiding this comment

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

And it does in the Reset function

Copy link
Author

Choose a reason for hiding this comment

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

In the comment Above

@Drach-Illya
Copy link
Author

The reset function is tied to the Submit button, to reset the values in the fields to an empty string and increase the count to prevent error messages from showing up when we submit the movie. There is no dedicated Reset button on the website.

Copy link

@StasSokolov1 StasSokolov1 left a comment

Choose a reason for hiding this comment

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

Awesome job, i'm approving it🔥

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.

2 participants