PhamNguyenDuyTien / df-frontend-2023

https://df-frontend-2023-alpha.vercel.app
0 stars 0 forks source link

Submission assignment 2 #2

Open PhamNguyenDuyTien opened 1 year ago

PhamNguyenDuyTien commented 1 year ago

Link demo: https://df-frontend-2023-assignment-2.vercel.app/

tienan92it commented 1 year ago

Hello @PhamNguyenDuyTien , great work!. Below is some feedback for your assignment.

Requirements

Final result: ✅ passed 70% of requirements

Feedback

  1. Should separate Pagination rendering and it's logic into a single component to make the code more readable, cleaner, and easy to maintain. https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/0cdc78671325d3e5d973c12656fe4dc42523d0c1/assignment-2/src/components/Bookstore/Bookstore.jsx#L70-L126

  2. This will reset storage every renders. Should wrap it inside a useEffect or handle in react context https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/0cdc78671325d3e5d973c12656fe4dc42523d0c1/assignment-2/src/App.js#L7

  3. When handle listeners with event as default param, don't need to create a inline function. https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/0cdc78671325d3e5d973c12656fe4dc42523d0c1/assignment-2/src/components/Bookstore/Bookstore.jsx#L138 should be:

    onChange={handleChangeSearchBooks}
  4. Should separate ListBooks into a independent component. https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/0cdc78671325d3e5d973c12656fe4dc42523d0c1/assignment-2/src/components/Bookstore/Bookstore.jsx#L200-L276

  5. Avoid prop drilling by using React Context or alternative state management libs. In your case, it's listBooks. Ref: Why prop drilling is a bad practice

  6. For a component, it should accept the related props only to prevent the component from re-rendering due to unnecessary data changes. For example: https://github.com/PhamNguyenDuyTien/df-frontend-2023/blob/0cdc78671325d3e5d973c12656fe4dc42523d0c1/assignment-2/src/components/Bookstore/Bookstore.jsx#L186-L192 Your Modal receive too many props that doesn't relate to a Modal's business

  7. When currentPage state is out of range of filtered listBooks (searching state), table will show nothing.