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

posts 1 #884

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

posts 1 #884

wants to merge 2 commits into from

Conversation

vetal-hovenko
Copy link

Copy link

@Viktor-Kost Viktor-Kost left a comment

Choose a reason for hiding this comment

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

Nice!
Some moments can be improved

break;
}
}
}, [name, email, body]);

Choose a reason for hiding this comment

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

There is no need to include 'name', 'email', and 'body' in the array of dependencies for the useCallback hook because they are not being used inside the hook.

}) => {
const [isFormOpened, setIsFormOpened] = useState<boolean>(false);

useMemo(() => {

Choose a reason for hiding this comment

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

You use useMemo instead of useEffect to reset the form state when a new post is selected. useMemo is for memoizing expensive calculations, useEffect is for side effects like resetting state.

src/App.tsx Outdated
const [commentsError, setCommentsError] = useState<boolean>(false);

useEffect(() => {
getUsers('/users')

Choose a reason for hiding this comment

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

Calling API endpoint directly in component. It's better to isolate API calls in separate services or utils.

src/App.tsx Outdated
{posts && posts.length > 0
&& (
<PostsList
posts={posts !== null ? posts : []}

Choose a reason for hiding this comment

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

Unnecessary null check. You already checked in the condition above that posts is not null and that its length is greater than 0.

Comment on lines 24 to 26
const [name, setName] = useState<string>('');
const [email, setEmail] = useState<string>('');
const [body, setBody] = useState<string>('');

Choose a reason for hiding this comment

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

I'd recommend you to use form fields as one state since this is one entity

Suggested change
const [name, setName] = useState<string>('');
const [email, setEmail] = useState<string>('');
const [body, setBody] = useState<string>('');
const [fields, setFields] = useState({
name: '',
email: '',
body: ''
});

Comment on lines 30 to 32
const [nameError, setNameError] = useState<boolean>(false);
const [emailError, setEmailError] = useState<boolean>(false);
const [bodyError, setBodyError] = useState<boolean>(false);

Choose a reason for hiding this comment

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

the same you can do with errors

}
}, [name, email, body]);

const formSubmit = useCallback((event: React.FormEvent<HTMLFormElement>) => {

Choose a reason for hiding this comment

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

reset a form after submitting

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.

3 participants