-
Notifications
You must be signed in to change notification settings - Fork 12
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
react-todo-list-challenge/Steven-Carcamo #11
base: master
Are you sure you want to change the base?
Conversation
import { createPortal } from "react-dom"; | ||
import TaskForm from "./TaskForm"; | ||
|
||
export default function CreateTaskFloatingButton() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest you separate the button from the form, maybe you can extract the logic here and move this to it's parent level that must be a better approach for handling this
src/components/CreateTaskForm.tsx
Outdated
}); | ||
|
||
const onSubmit = (data: TaskFormData) => { | ||
const newTask = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const newTask = { | |
const newTask = { | |
id, | |
...data, | |
} |
src/components/CreateTaskForm.tsx
Outdated
dueDate: data.dueDate | ||
}; | ||
|
||
const existingTasks = JSON.parse(localStorage.getItem("tasks") || "[]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can define this outside of the function right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll work on this improvements tomorrow
|
||
addTask(newTask); | ||
|
||
reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can define a helper function called onSuccess and wrap this two functions there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just what I thought, actually there is still some refactor I want to do
errors={errors} | ||
register={register} | ||
dataItem={task} | ||
formMode="edit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
|
||
export default function SearchTask() { | ||
const [searchTerm, setSearchTerm] = useState<string>(""); | ||
const [termDebounced] = useDebounce(searchTerm, 500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job
setSearchTerm(e.currentTarget.value); | ||
}; | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if a useEffect is really needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll figure it out
src/components/TaskCard.tsx
Outdated
const deleteTask = useTasksStore((state: any) => state.deleteTask); | ||
|
||
const handleDelete = (taskId: string) => { | ||
const existingTasks = JSON.parse(localStorage.getItem("tasks") || "[]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using this multiple times, I think you also defined this variable in your store, why are you not using it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got what you mean, I'll check it out
|
||
|
||
export default function TaskList() { | ||
const tasks = useTasksStore((state: TasksStore) => state.tasks) as Task[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the cast needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already defined the types in the zustand store
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to remove it, this is no longer needed
src/config/validation/taskSchema.ts
Outdated
import { z } from "zod"; | ||
|
||
export const taskSchema = z.object({ | ||
id: z.string().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be
z.string().uuid() if I'm not wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I'll change it
id: z.string().optional(), | ||
title: z.string().min(5, { message: "Task title is required" }).max(30, { message: "Task title is too long" }), | ||
priority: z.enum(["Urgent", "High", "Normal", "Low"]), | ||
storyPoints: z.coerce.number().min(1, { message: "Number must be a number between 1 and 10" }).max(20, { message: "Number must be a number between 1 and 20" }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why coerce is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that input type="number" when is accessed by the form, for some reason is taking it as string, so this parses to string.
src/stores/tasks.ts
Outdated
import { create } from "zustand"; | ||
import { Task, TasksStore } from "../types/Task"; | ||
|
||
export const useTasksStore = create<TasksStore>((set) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zustand has a middleware called persist that you can use to avoid doing all the JSON.parse localStorage... stuff
https://arc.net/l/quote/cbgffbxe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check it out
src/components/TaskFormLayout.tsx
Outdated
import { Task, TaskFormMode } from "../types/Task"; | ||
|
||
interface FormLayoutProps { | ||
handleSubmit: (data: any) => any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any usage!
No description provided.