mirageruler / df-frontend-2023

learning FE stuffs from dwarves
https://df-frontend-2023-rust.vercel.app
0 stars 1 forks source link

Submission for assignment 2 #2

Open mirageruler opened 1 year ago

mirageruler commented 1 year ago
tienan92it commented 1 year ago

Hello @mirageruler , well done! Below is some feedback for your assignment.

Requirements

Final result: ✅ passed 60% of requirements

Feedback

  1. Move static data outside of the component or put them into a constants file to prevent data re-created from rendering. https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L6-L16 https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L31

  2. What is the purpose of Array.isArray in if (lsBook && Array.isArray)? It's a function :/ https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L45

  3. After initializing, if there are no books in local storage, books state still empties because it isn't updated after localStorage.setItem("books", JSON.stringify(books)); https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L43-L50 Suggestion:

    useEffect(() => {
        let lsBook = JSON.parse(localStorage.getItem("books"));
        // Initialize mock data if there are no books in storage
        if (!Array.isArray(lsBook)) {
            lsBook = books;
            localStorage.setItem("books", JSON.stringify(books));
        }
        setData(lsBook);
    }, []);
  4. Don't assign a state to a mutated variable. It will lead to unexpected bugs if you change the mutated variable. https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L33-L35 In this case, right after you push the new book into lsBooks, data state is changed too. https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L66-L67

  5. It's better if this code is separated into another component like AddBookForm. https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L160-L186

  6. Unnecessary to create an inline function for the listener. Just onClick={onCloseModal} if it doesn't need any other params like onClose={() => onCloseModal("delete")} https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L191C65-L191C77

  7. Research controlled and uncontrolled components to improve the performance of input and prevent the entire component from re-rendering. Basically useRef to handle input form data. https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L165C40-L165C53

  8. Should separate App into independent components like: BookTable, Pagination,... https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L118-L143

  9. Mange books by unique id to avoid bug from deleting books with same title. https://github.com/mirageruler/df-frontend-2023/blob/0237fa0a4f4e055130fa2b1c22fa30fb79450662/assignment-2/src/App.js#L75-L82