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

[박상준] Week11 #366

Merged

Conversation

sj0724
Copy link
Collaborator

@sj0724 sj0724 commented Apr 28, 2024

요구사항

기본

  • 폴더 페이지에 필요한 “폴더 이름 변경”, “폴더 추가”, “폴더 공유”, “폴더 삭제”, “링크 삭제”, “폴더에 추가” 모달을 만들어 주세요.
  • “폴더 공유” 모달의 기능은 클릭시 ‘{호스트 주소}/shared/{currrentFolderId}’ 로 접속 가능한 주소를 공유 또는 클립보드에 복사 가능하게 해주세요.
  • 팀내 2 ~ 3명이 짝을 지어 페어프로그래밍으로 여러 모달을 만들 때 사용할 모달 컴포넌트를 만들어 주세요.
  • 카드에 있는 케밥 버튼을 클릭할 경우 보여야 할 팝오버를 만들어 주세요.

주요 변경사항

  • 모달 구현

스크린샷

image

멘토에게

  • 여러개의 모달을 modals라는 컴포넌트에서 불러와서 모달 타입에 따라 조건부 렌더링을 하고 있는데 이런 방식이 괜찮을까요?

@sj0724 sj0724 requested a review from kiJu2 April 28, 2024 14:33
@sj0724 sj0724 self-assigned this Apr 28, 2024
@sj0724 sj0724 added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Apr 28, 2024
@sj0724 sj0724 changed the title Part2 박상준 week11 [박상준] Week11 Apr 28, 2024
@kiJu2
Copy link
Collaborator

kiJu2 commented May 1, 2024

수고 하셨습니다 ! 위클리 미션 하시느라 정말 수고 많으셨어요.
학습에 도움 되실 수 있게 꼼꼼히 리뷰 하도록 해보겠습니다.

@kiJu2
Copy link
Collaborator

kiJu2 commented May 1, 2024

여러개의 모달을 modals라는 컴포넌트에서 불러와서 모달 타입에 따라 조건부 렌더링을 하고 있는데 이런 방식이 괜찮을까요?

코드리뷰할 때 한 번 확인해볼게요 ! 😊

Comment on lines +9 to +13
const { data } = await axios.get('/sample/folder');
for (let i = 1; i < data.folder.count; i++) {
data.folder.links[i].image_source = data.folder.links[i].imageSource;
delete data.folder.links[i].imageSource;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try ... catch를 추가하고 forEach 문을 사용해볼 수 있습니다 !:

Suggested change
const { data } = await axios.get('/sample/folder');
for (let i = 1; i < data.folder.count; i++) {
data.folder.links[i].image_source = data.folder.links[i].imageSource;
delete data.folder.links[i].imageSource;
}
try {
const { data } = await axios.get('/sample/folder');
if (data && data.folder && data.folder.links) {
data.folder.links.forEach((link, index) => {
if (link.imageSource) {
link.image_source = link.imageSource;
delete link.imageSource;
}
});
}
return data;
} catch (error) {
console.error('Error fetching sample folder:', error);
throw error; // 에러를 던져서 호출한 측에서도 처리할 수 있도록 함
}

setTotalBtn(false);
};

const setTotal = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 함수는 핸들러의 역할을 수행하고 있으므로 다음과 같이 네이밍을 변경해보는건 어떨까요?:

Suggested change
const setTotal = () => {
const handleClickTotalButton = () => {

const handleMenuClick = (index) => {
const booleanArr = [...link].fill(false);
booleanArr[index] = true;
setLinkSelected(booleanArr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 이해하기로는.. 원래 boolean이었는데 boolean Array로 변경되는걸까요?

Comment on lines +15 to +17
const booleanArr = [...link].fill(false);
booleanArr[index] = true;
setLinkSelected(booleanArr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 클릭한 메뉴를 식별하는 코드라면 배열이 아닌 문자열로 해도 될 것 같아요 ! 😊

);
}

function AddModal({ setModal, link }) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다음과 같이 재설계해보면 어떨까요?

Suggested change
function AddModal({ setModal, link }) {
function AddModal({ open, onClose, link }) {

AddModal이 원하는 것은 CloseIcon을 눌렀을 때 핸들러이므로 어떤 핸들러 함수를 전달하는게 더욱 명확할 것으로 보여요 😊

import * as S from '../DeleteModal/DeleteModal.styled';
import closeIcon from '../../../assets/close.svg';

function DeleteLinkModal({ setModal }) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 마찬가지입니다 !

Suggested change
function DeleteLinkModal({ setModal }) {
function DeleteLinkModal({ open, onClose }) {

@@ -0,0 +1,91 @@
import styled from 'styled-components';

export const Background = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

길어져서 코드를 분리하셨군요 😊👍

import ShareModal from '../ShareModal/ShareModal';
import * as S from './Modals.styled';

function Modals({ modalType, setModal, folderName, link }) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기가 질문주셨던 부분이군요.

Modals 컴포넌트를 만들어서 중앙집중화 시키는 것 보다는, 커스텀 훅(useModal)을 만들어서 관리하는 것도 좋은 방법일 것 같아요.
제가 작성했던 코드 공유드릴게요 !:

import React, { useContext, useReducer } from 'react';

const ModalContext = React.createContext();

const modalReducer = (state, action) => {
  switch (action.type) {
    case 'OPEN_MODAL':
      return { ...state, [action.modalName]: true };
    case 'CLOSE_MODAL':
      return { ...state, [action.modalName]: false };
    default:
      throw new Error(`Unhandled action type: ${action.type}`);
  }
};

export const ModalProvider = ({ children }) => {
  const [modalState, dispatch] = useReducer(modalReducer, {});

  const openModal = (modalName) => {
    dispatch({ type: 'OPEN_MODAL', modalName });
  };

  const closeModal = (modalName) => {
    dispatch({ type: 'CLOSE_MODAL', modalName });
  };

  return (
    <ModalContext.Provider value={{ modalState, openModal, closeModal }}>
      {children}
    </ModalContext.Provider>
  );
};

export const useModal = () => useContext(ModalContext);

그리고 다음과 같이 사용합니다:

import React from 'react';
import { ModalProvider, useModal } from './ModalContext'; // 경로는 ModalContext 파일의 위치에 따라 다를 수 있습니다.

const App = () => {
  return (
    <ModalProvider>
      <MainComponent />
    </ModalProvider>
  );
};

const MainComponent = () => {
  const { modalState, openModal, closeModal } = useModal();

  return (
    <div>
      <h1>Welcome to the Modal Example</h1>
      <button onClick={() => openModal('login')}>Open Login Modal</button>
      <button onClick={() => openModal('signup')}>Open Signup Modal</button>
      {modalState.login && (
        <Modal modalName="login" onClose={() => closeModal('login')}>
          <p>This is the login modal</p>
        </Modal>
      )}
      {modalState.signup && (
        <Modal modalName="signup" onClose={() => closeModal('signup')}>
          <p>This is the signup modal</p>
        </Modal>
      )}
    </div>
  );
};

const Modal = ({ modalName, onClose, children }) => (
  <div className="modal">
    <div className="modal-content">
      <h2>{modalName} Modal</h2>
      {children}
      <button onClick={onClose}>Close</button>
    </div>
  </div>
);

export default App;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

또한, 레이아웃이 많이 다르다면 각 기능에 맞는 Modal들을 만들어두는 것은 당연합니다 ! 모달의 개수가 늘어난다고 부담갖지 않으셔도 됩니다 😊

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹은 compound pattern을 통해서 Modal, ModalBody, ModalHeader 등의 ui를 만들어두고 각 목적에 맞는 모달들을 합성하는 패턴도 추천드려요 !

compound pattern

import { useEffect, useState } from 'react';
import { getFolder } from '../api/api';

function useGetFolderList(userId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳굳 ! API 별로 커스텀 훅을 만드셨군요 👍

@kiJu2
Copy link
Collaborator

kiJu2 commented May 1, 2024

훌륭합니다 상준님 ! 수고 정말 많으셨어요.
프로젝트 끝나고 해이해질 수도 있었을텐데 바로바로 과제를 수행하셨군요 👍👍
모달과 관련해서는 한 번 고민해보시면 좋을 것 같아요 😊

@kiJu2 kiJu2 merged commit e594b45 into codeit-bootcamp-frontend:part2-박상준 May 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants