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

Clone task #772

Merged
merged 13 commits into from
Jan 10, 2024
Merged

Clone task #772

merged 13 commits into from
Jan 10, 2024

Conversation

negreirosleo
Copy link
Contributor

This PR's adds a button to the TaskList that when clicked it clones the task data to the create task form

Screenshots

Screencast.from.2024-01-08.11-42-49.webm

Copy link
Collaborator

@dmtrek14 dmtrek14 left a comment

Choose a reason for hiding this comment

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

For cloning, the times are not copied in the existing application. The user will have to set them anyway or else it would fail validation as overlapping, so it is probably best to clear them here, too, to save the user a step.

I am also still noticing the issue where the tasks are not sorted correctly on the right side after I add a task.
Screenshot from 2024-01-09 08-51-24

I have also noticed that once a task is submitted, the form does not clear. Should we clear the form on successful submission? Now that we have clone (which fills the form), it may start to get confusing to users if the form is essentially always filled. For instance, I started the app to test and created a task for today so that I could clone it. Because the form didn't clear after I saved the task, it was hard to tell if clone was initially doing anything. What do you think?

@negreirosleo
Copy link
Contributor Author

@dmtrek14 I agree with you on the form clear and the dates, it creates a better UX

@negreirosleo negreirosleo force-pushed the clone-task branch 2 times, most recently from 648e648 to 051ee1b Compare January 9, 2024 22:41
@negreirosleo
Copy link
Contributor Author

@dmtrek14 I also took the opportunity and added a loading state to the submit button 👍

@negreirosleo negreirosleo requested a review from dmtrek14 January 9, 2024 22:42
Copy link
Member

@anarute anarute left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Collaborator

@dmtrek14 dmtrek14 left a comment

Choose a reason for hiding this comment

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

Looks good - thanks for the changes!

Since this is a common behaviour to all forms it is being moved to the
general hook to make it DRYer
Since we need the form logic in different places of the page, a provider
will be use to avoid prop drilling and sending the required parts of the
form logic to where it is needed
Since that piece of code is only used by the CreateTask component it is
being isolated to only be called where it is needed.
it unifies the check for overlapping tasks and test the schema
It moves the validation from the handleSubmit to the editTask api call
and removes the tasks prop drilling to get it directly on the
useEditTask react query hook
It should return Promise<Template> instead of  Promise<{data: Template}>
Error.captureStackTrace is a V8 api that is not common to all browsers,
this check is to ensure that it will not cause error in browsers that do
not support that api
This changes simplifies the useCreateTask hook and moves the validation
closer to the api call, centralizing the error handling for the endpoint
and reducing the logic on handleSubmit.
TaskBox tests were included in the TaskList, since the component is
growing in complexity it should have it's own tests.
@negreirosleo negreirosleo merged commit 3a061c0 into main Jan 10, 2024
2 checks passed
@negreirosleo negreirosleo deleted the clone-task branch January 10, 2024 16:55
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