Todo app built with t3 stack

Community feedback

• 

  j0sephh123•140

  @j0sephh123Posted over 1 year ago

  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.status in src/modules/todos/views/TodosView.tsx, i have a small idea to create a custom hook. Since you are using router.query.status in 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 useTodos hook.

  export function useTodos(status?: TodoStatus) {
    const { isLoading, data } = api.todos.getAll.useQuery({ status });
  
    return { isLoading, todos: data };
  }
  

  0

  ---

  

  jakubfiglak•400

  @jakubfiglakPosted over 1 year ago

  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

  1

  

  j0sephh123•140

  @j0sephh123Posted over 1 year ago

  @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

  0