henrynguyen6677 / df-frontend-2023

0 stars 0 forks source link

[assignment-2] Submission for assignment 2 #2

Open henrynguyen6677 opened 1 year ago

henrynguyen6677 commented 1 year ago

https://df-frontend-2023-ojhr.vercel.app/

chinhld12 commented 1 year ago

Hi @henrynguyen6677 ,

Requirements

Final result: ✅ passed

Feedback

  1. The light/dark mode description should dynamically reflect the currently applied theme, as it always displays as "Light mode" even when the theme mode has been toggled to dark. To improve the user experience, you should store the selected preferred theme in localStorage. This way, the user's theme preference can be remembered and applied consistently across sessions.
image
  1. Searching the book name should support the case-sensitive.

  2. Should validate input before adding a new book

  3. This process should be refactored into a pure function to obtain the initial data, and a state should be used to store the book data instead of the current approach. This change is necessary because React cannot automatically re-render based on changes in localStorage. And use with localStorage operations should be handled with error handlings too.

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/components/base/main.component.jsx#L14-L15

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/seed/bookstore.js#L4-L39

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/App.js#L19-L20

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/components/overlays/add-book.overlay.jsx#L16-L20

  1. It seems like there are a couple of issues in your React code:

Example you can create context like this:

import { createContext } from "react";

const noop = () => {}

export const BookRowsContext = createContext({
  rows: [],
  setRows: noop,
  setStart: noop,
  start: 1,
  deleteRow: {},
});

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/contexts/mode.context.js#L2-L6

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/contexts/book-rows.context.js#L2-L9

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/contexts/overlay.context.js#L2-L6

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/components/toogle/toogle.component.js#L7-L14

.

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/App.js#L21-L22

  1. Naming variables is too generic, and does not accurately reflect the purpose and meaning of the code they represent.
    • start -> page
    • rows -> books
    • AddBookOverlay -> AddBookFormModal
    • DeleteBookOverlay -> DeleteBookModal
    • ...

https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/App.js#L20-L21

  1. Should be a button element for clickable elements https://github.com/henrynguyen6677/df-frontend-2023/blob/de3ea3a4f03222b57af5475f53bd946833c1db06/assignment-2/src/components/toogle/toogle.component.js#L19-L20
henrynguyen6677 commented 1 year ago

@chinhld12 Thank you for your feedback. I have a question about ticket Standard use of React component & state. What is the standard to follow?

chinhld12 commented 1 year ago

@henrynguyen6677

To enhance your codebase, you should address several issues:

...

You can see more here:

henrynguyen6677 commented 1 year ago

@chinhld12 Please take your time to review this PR #4 for refactor of assignment 2. With some main improvements:

Add capture: image Add link: https://df-frontend-2023-ojhr.vercel.app/

Thank you for your efforts

chinhld12 commented 1 year ago

Looks better than the previous one but still has some bugs:

  1. Should automatically change to page 1 when page 2 is empty after deleting all book records image

  2. The Page button should be available for the user to click on the padding area too, not only the number image

  3. Since these are functional methods for handling actions, you should use camelCase instead of PascalCase https://github.com/henrynguyen6677/df-frontend-2023/blob/edf3ac0d696ac4d911702ee15ad84c6427a81910/assignment-2/src/utils/localstore.js#L6-L20

  4. Typo ToogleComponent -> ThemeToggleInput

  5. Seem you have not fixed this: https://github.com/henrynguyen6677/df-frontend-2023/blob/edf3ac0d696ac4d911702ee15ad84c6427a81910/assignment-2/src/contexts/books.context.js#L3-L14

henrynguyen6677 commented 1 year ago

@chinhld12 Thank you for your feedback. I have some updates on it. Please take your time to review this PR #11 for refactoring assignment 2.

Improvements:

Add video:

https://github.com/henrynguyen6677/df-frontend-2023/assets/52127765/2f97d78e-a0da-401e-9651-134867d22f41

Link: https://df-frontend-2023-ojhr.vercel.app/

Thank you for your efforts