dnhn / df-fe-23

https://dffe23-dn-7.vercel.app
1 stars 0 forks source link

Assignment 4 #8

Open dnhn opened 1 year ago

dnhn commented 1 year ago

https://dffe23-dn-4.vercel.app

ngolapnguyen commented 1 year ago

Great work 👍

Requirements

Result: ✅

Feedbacks

  1. (Nitpick) As a common pattern, IButton interface should be in the same folder as the Button component. E.g. something like this:
components/
  Button/
    Button.tsx
    types.ts // <-- put it here
    ...
  1. Minor, methods like setSearch, setPageIndex, etc. (from useState) will not trigger rerender, so there's no need to memoize them:
image
  1. We should be able to improve url params syncing logic by centralizing the params in one place and use a dedicated listener to update them to the url:
const [params, setParams] = useState({
  q: '',
  page: 1,
  pageSize: 5,
  ...
})

useEffect(() => {
  push(...)
}, [params])

You can also look for 3rd party packages such as query-string to help with converting an object into query string.

You should also be able to extract the params management logic into a reusable hook 🤔

  1. Current approach to centralize dialog rendering logic is fine, but might get harder and harder to manage once the app grows. I'd suggest you look into portal as a way to render dialog on demand from everywhere, and use a more direct approach to manage dialog.

For example, we can do something like:

<>
  <button onClick={openDeleteBookDialog}>Delete</button>
  <DeleteBookDialog open={open} ... />
</>

Basically what we normally see in UI library like MUI. This should help you cut out the BooksDialogContext and keep the codebase a bit cleaner.