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

Submission for assignment 5 #5

Open
nguyenhoaikhang37 opened this issue Oct 8, 2023 · 1 comment
Open

Submission for assignment 5 #5

nguyenhoaikhang37 opened this issue Oct 8, 2023 · 1 comment

Comments

@nguyenhoaikhang37
Copy link
Owner

nguyenhoaikhang37 commented Oct 8, 2023

/ Demo link
https://df-frontend-2023-njoi.vercel.app

// Any notes for reviewers
If the code lacks clarify or if there are any issue with the application, please don't hesitate to provide feedback. I'm open to suggestions and eager to improve. Thanks a lot <3

@ngolapnguyen
Copy link

ngolapnguyen commented Oct 15, 2023

Requirements

  • All core functionalities present
  • Edit form
  • Login page
  • Forms built by react-hook-form
  • Validation by zod
  • Bonus: Password Strength Meter

Result: ✅

Feedbacks

Overall good migration & form implementation 👍 Some nitpicks:

  1. Users should be redirected into the dashboard after login:
image

There's something buggy here. First time I logged in, I wasn't redirected. It worked on the second time, though 🤔

  1. You should consider moving these logic outside of the forms:
image

Current implementation assumes that by going back once, you're back to homepage. This might hold true for this project, but it's not a good practice to build a component with assumptions. It reduces the flexibility of the component.

By moving the logic out, we leave the responsibility to the parent components:

<DeleteBookDialog
  ...
  onDelete={() => {
    deleteBook();
    push('/');
  })
/>

Now you can use DeleteBookDialog everywhere & be able to control the navigation. Or you can keep the deleteBook call inside, and use an onAfterDelete props to handle what happens after:

<DeleteBookDialog
  ...
  onAfterDelete={() => {
    push('/');
  })
/>
  1. Some files' locations are... questionable:
image

You can keep these in the same place as the pagination component. E.g:

src/components/common/pagination
  pagination.tsx
  pagination.css
  ...
  1. This useMemo will run every renders, because handleLogin and handleLogout are not memoized:
image

Some points:

  • Eslint has a rule to prevent this (I don't remember exactly, but probably eslint-plugin-react-hooks provide the rule?)
  • You can wrap those methods in useCallback to memoize them
  • Normally we shouldn't have to memoize a context provider's value, you memoize the methods & stuff sufficiently 🤔
  1. ... (Please refer to my feedbacks for previous assignments)

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

No branches or pull requests

2 participants