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

almost Fully functional Todo App🎀 #803

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

Conversation

polinavafik
Copy link

@polinavafik polinavafik commented Sep 26, 2023

DEMO LINK 🎀

@polinavafik polinavafik changed the title Fully functional Todo App🎀 almost Fully functional Todo App🎀 Sep 27, 2023
Copy link

@voronine voronine left a comment

Choose a reason for hiding this comment

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

Ehh, good work, but a lot of test errors. Go to QN, they helped me yesterday.

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.

Looks great 🔥
Left some comments to improve your solution

src/App.tsx Outdated
setTodos,
error,
setError,
} = useContext(TodoContext);

Choose a reason for hiding this comment

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

You can create hooks for contexts so you don't have to pass context every time.

const useTodo = () => useContext(TodoContext);

src/App.tsx Outdated
.then(setTodos)
.catch(() => {
setError(CurrentError.LoadingError);
throw new Error();

Choose a reason for hiding this comment

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

You don't need to throw an error in this case, as in this case, you don't catch this error
image

})
.catch(() => {
setError(CurrentError.DeleteError);
throw new Error();

Choose a reason for hiding this comment

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

The same, just set an error to notify user, it's enough in this case

Comment on lines 109 to 115
return Promise.resolve();
}

if (!newTodoTitle.trim()) {
setError(CurrentError.EmptyTitleError);

return Promise.reject();

Choose a reason for hiding this comment

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

You don't need to reject or resolve in this cases, just return in case you don't need to proceed

Copy link
Author

Choose a reason for hiding this comment

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

I had a problem without resolve reject since I have typization handleTodoRename: (todo: Todo, newTodoTitle: string) => Promise<void>

but I added | void, to it and now it passes 😅 how I didn't think of that before

setIsLoading(true);
setProcessingTodoIds(prevState => [...prevState, todo.id]);

// eslint-disable-next-line consistent-return

Choose a reason for hiding this comment

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

Don't disable eslint, I think in this case you actually don't need to return

Copy link
Author

Choose a reason for hiding this comment

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

If I don't have return, then when you edit todo, after edit on load it shows the previous version of the title and then shows the new one (after load)

but with return, everything works as it should...

Comment on lines 51 to 64
if (error === CurrentError.UpdateError) {
setError(CurrentError.UpdateError);
throw new Error();
}

if (error === CurrentError.DeleteError) {
setError(CurrentError.DeleteError);
throw new Error();
}

if (error === CurrentError.EmptyTitleError) {
setError(CurrentError.EmptyTitleError);
throw new Error();
}

Choose a reason for hiding this comment

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

You don't need this as you set errors previously in you handlers, so you don't need to throw errors and handle them here one more time

const inputField = useRef<HTMLInputElement>(null);

const isAllCompleted = useMemo(() => (
todos.every(el => el.completed)

Choose a reason for hiding this comment

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

Suggested change
todos.every(el => el.completed)
todos.every(el => todo.completed)


return (
<section className="todoapp__main" data-cy="TodoList">
<TransitionGroup>

Choose a reason for hiding this comment

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

🔥

return (
<section className="todoapp__main" data-cy="TodoList">
<TransitionGroup>
{todos?.map(todo => (

Choose a reason for hiding this comment

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

todos is required prop in accordance with type

Comment on lines 8 to 12
let preparedTodos = [...todos];

if (selectedFilter !== TodoFilter.All) {
preparedTodos = preparedTodos.filter(({ completed }) => {
switch (selectedFilter) {

Choose a reason for hiding this comment

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

Suggested change
let preparedTodos = [...todos];
if (selectedFilter !== TodoFilter.All) {
preparedTodos = preparedTodos.filter(({ completed }) => {
switch (selectedFilter) {
if (selectedFilter !== TodoFilter.All) {
return todos.filter(({ completed }) => {
switch (selectedFilter) {

filter returns new array so you don't need create a copy

Copy link
Author

Choose a reason for hiding this comment

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

😅ooopsss

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 🚀

Comment on lines +77 to +94
<div
data-cy="ErrorNotification"
className={classNames(
'notification',
'is-danger',
'is-light',
'has-text-weight-normal',
{ hidden: !error },
)}
>
<button
data-cy="HideErrorButton"
type="button"
className="delete"
onClick={() => setError(CurrentError.Default)}
/>
{error}
</div>

Choose a reason for hiding this comment

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

Create a component ErrorNotification and move the logic for errors to it too

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