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

Task solution (read description) #2894

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

Conversation

AnderZerfall
Copy link

My tests are completely broken because of a mysterious issue — I'm still getting the error "Failed to fetch the module...". So, idk, if it passes the test or not. I compared my code to others and it should work fine...
At least the demo seems to be fine.

DEMO LINK

@AnderZerfall
Copy link
Author

Some checks were not successful. I'll fix that asap

Copy link

@BudnikOleksii BudnikOleksii 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 👍
But pay attention to naming

src/App.tsx Outdated
}));

export const App: React.FC = () => {
const [listTodo, updateTodoList] = useState(todos);

Choose a reason for hiding this comment

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

Suggested change
const [listTodo, updateTodoList] = useState(todos);
const [todoList, setTodoList] = useState(todos);

Developers have a convention on how to name use state https://react.dev/reference/react/useState
image

src/App.tsx Outdated
Comment on lines 26 to 29
updateTodoList([
...listTodo,
{ userId, id, title, user: getUserById(userId), completed: false },
]);

Choose a reason for hiding this comment

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

When you set state depending on the previous one use functional setState

Comment on lines 42 to 43
setTitle(title.replace(/[^a-zA-Z0-9 ]/g, '')); // I just didn't want to create a cycle for deleting characters
} // So I googled this expression

Choose a reason for hiding this comment

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

If you have a regex create a variable for it and put comments close to it

Comment on lines 13 to 14
const [titleHasError, setTitleError] = useState(false);
const [userHasError, setUserError] = useState(false);

Choose a reason for hiding this comment

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

Fix naming, in accordance with convention and boolean variables should start from is or has

resetForm();
};

const titleValidation = (title: string) => {

Choose a reason for hiding this comment

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

Suggested change
const titleValidation = (title: string) => {
const validateTitle = (title: string) => {

Function name should start from verb

Comment on lines 56 to 60
onChange={event => {
setTitle(event.target.value);
titleValidation(event.target.value);
setTitleError(false);
}}

Choose a reason for hiding this comment

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

  • Create a function instead of passing anonim one to onChange
  • You setTitle in the validation function

Comment on lines 72 to 75
onChange={event => {
setUserId(+event.target.value);
setUserError(false);
}}

Choose a reason for hiding this comment

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

Create a function

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