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

first solution with issues #846

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

Conversation

vensecue
Copy link

@vensecue vensecue commented Oct 1, 2023

Copy link

@choeqq choeqq left a comment

Choose a reason for hiding this comment

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

If I mark Todo as completed only this one todo should start loading - in your case every single todo from the list starts loading - please try to fix it

src/App.tsx Outdated
return visible;
};

const handleSubmit = (event: { preventDefault: () => void }) => {
Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

I think you should consider splitting your component's logic into multiple custom hooks - App component grew really big, since you're passing a lot of functions and states to children components, maybe consider global state with context? 🤔

that't not mandatory, but I think it would be a nice practice

type FooterProps = {
counter: number;
filter: Filter;
setFilter: (filter: Filter) => void;
Copy link

Choose a reason for hiding this comment

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

you can copy type of setState function, when you hover over that function in VSC, it will be more specific than void

Copy link

@choeqq choeqq left a comment

Choose a reason for hiding this comment

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

Waiting for any updates @vensecue 👀

Copy link

@choeqq choeqq left a comment

Choose a reason for hiding this comment

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

Waiting for any updates @vensecue 👀

Copy link

@choeqq choeqq left a comment

Choose a reason for hiding this comment

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

Waiting for any updates @vensecue 👀

Copy link

@pawel-peksa pawel-peksa 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, It looks like the App is almost working as expected even though a number of tests still fails.
I've added two comments regarding the improvement of app functionality.

Comment on lines 47 to 49
const handleBlur = () => {
setIsEditing(false);
};

Choose a reason for hiding this comment

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

On blur we should:

  • Do nothing only when title has not changed.
  • Update title when it has changed
  • Remove Todo when title left empty
    This logic is missing here

Comment on lines 94 to 101
<button
type="button"
className="todo__remove"
data-cy="TodoDelete"
onClick={() => handleDelete(todo.id)}
>
×
</button>

Choose a reason for hiding this comment

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

Should be hidden when isEditing is true

Copy link

@pawel-peksa pawel-peksa left a comment

Choose a reason for hiding this comment

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

Application works as expected. Good job! 🥳

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