@j0sephh123
Posted
Great job, looks very clean and maintainable. Works fine on the deployed url, haven't started it locally due to the need of postgres. I really like how you have used react-query, modules folder, hooks with optimistic updates and rollback.
- Small idea for Typescript:
// src/layout/AppLayout.tsx
type LayoutContentWrapperProps = {
className?: string;
} & PropsWithChildren;
…
export const AppLayout = ({ children }: PropsWithChildren) => {
- Seeing that you cast
router.query.statusinsrc/modules/todos/views/TodosView.tsx, i have a small idea to create a custom hook. Since you are usingrouter.query.statusin 2 files you may be able to abstract a bit.
function useRouterQueryStatus<T>() {
const router = useRouter();
return router.query.status as T;
}
export const TodosView = ({ className }: TodosViewProps) => {
const status = useRouterQueryStatus<TodoStatus>();
- Slightly opinionated idea for
useTodoshook.
export function useTodos(status?: TodoStatus) {
const { isLoading, data } = api.todos.getAll.useQuery({ status });
return { isLoading, todos: data };
}
@jakubfiglak
Posted
Hey @j0sephh123 thank you very much for taking the time to look at my work, really appreciate it! I like the idea with useRouterQueryStatus hook, will implement it. It doesn't even have to accept a generic in my opinion as it's probably always gonna be of type TodoStatus.
As for the AppLayout props suggestion - I'm not using PropsWithChildren because children is optional in this type and to me it doesn't make much sense to use a layout component without children so I'd rather have it required. Oh and PropsWithChildren is a generic type so probably the best way of leveraging it would be something like this:
// src/layout/AppLayout.tsx
type LayoutContentWrapperProps = {
className?: string;
};
…
export const AppLayout = ({ children, className }: PropsWithChildren<LayoutContentWrapperProps>) => {
But still, I believe typing children manually is the best option in this scenario.
useTodos suggestion - sorry but I need to disagree again, your suggestion limits the properties naturally returned by React Query (what if I wanted to use error property, refetch the query manually in my component or anything else?). Also, I kind of like the convention of using RQ hooks like this:
const todos = useTodos()
console.log(todos.isLoading)
console.log(todos.data)
This approach leverages the natural namespace and saves me for example from needing to rename isLoading if I use multiple hooks in one component. And it just feels right to me :D
@j0sephh123
Posted
@jakubfiglak
Yeah, good point. Makes sense for useTodos and PropsWithChildren.
Will definitely "steal" the idea to use the authentication library for future projects :)
Marked as helpful
