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

MISSION8 / 강하은 #49

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kanghaeun
Copy link
Collaborator

1. 구현 모습

2. 해결 과정

  • App 컴포넌트에서는 Supabase에서 받아온 데이터를 저장하였습니다. 그리고 Main과 ContentDisplay라는 두 개의 큰 컴포넌트로 나누어 렌더링하였습니다. 모달을 최근에 너무 많이 사용했기 때문에, 다른 디자인 방식을 고려하다가 크게 두 컴포넌트로 나누기로 결정했습니다.

  • Main 컴포넌트는 화면 왼쪽에 위치하며, header와 list 컴포넌트로 구성되어 있습니다. header에는 글쓰기 버튼 기능을 추가하여 모달을 띄우도록 하였고, 모달 관련 코드는 별도의 컴포넌트로 분리하였습니다. list 컴포넌트 내부에는 리스트와 페이지네이션 기능이 구현되어 있는데, 페이지네이션 기능도 별도의 컴포넌트로 만드는 것이 좋을지 고민 중입니다.

  • list 컴포넌트 내부의 카드 데이터를 기반으로 카드 디자인을 설계하는 코드가 중복되었기 때문에, 이를 해결하기 위해 ListItem이라는 공통 컴포넌트를 만들었습니다. 그리고 list에 있는 카드를 클릭하면 ContentDisplay 컴포넌트에 해당 데이터를 전달하도록 App 컴포넌트에 handlePostClick 함수를 설정하였습니다.

  • ContentDisplay 컴포넌트에서는 App 컴포넌트에서 받은 데이터를 기반으로 화면을 렌더링하고, 삭제와 생성 기능도 Supabase를 이용하여 구현하였습니다.


3. To 리뷰어에게

  • 코드를 구현하면서 중복되는 코드들을 제거하려고 노력했지만 제가 발견하지 못한 코드들이 많은 거 같아요. 코드 중복을 줄이기 위해서 어떤 방법들을 고려하시는지 궁금합니다!
  • ContentDisplay에 넘겨줄때 해당 데이터를 넘겨주어 본문 view를 보여주어야 할때 데이터를 보내주어야 하는데 저는 title,tag등의 전체 데이터를 넘겨주었어요. 아무래도 전달하기 편하고 바로 적용할 수 있기 때문에 이런식으로 넘겨주었는데 생각해보니 데이터가 자주 변경되거나 하면 문제가 발생할 수 있겠다라는 생각이 드네요.. 이럴때는 id만 넘겨주어 해당 id값을 다시 찾아 데이터를 불러오는 게 나을까요? 아니면 별 차이 없는지 궁금합니다!
  • 한 컴포넌트에 너무 많은 코드를 집어넣지 않으려고 노력했습니다. 보통 한 컴포넌트에 몇 줄을 넣으시고 컴포넌트에 너무 많은 코드를 집어넣지 않으려는 노력 또는 기준이 있을까요?
  • 추가적으로 해보면 좋을 기능들이 있을까요?

이번 스터디를 통해 많은 것을 배우고 성장할 수 있었습니다. 마지막 미션까지 함께해주셔서 정말 감사합니다!!

@kanghaeun kanghaeun added the mission 미션 입니다! label Jun 26, 2024
@kanghaeun kanghaeun self-assigned this Jun 26, 2024
Copy link

vercel bot commented Jun 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
donut-study ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 26, 2024 9:27am

Copy link
Collaborator

@SingTheCode SingTheCode left a comment

Choose a reason for hiding this comment

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

하은님께서 질문하신 것 먼저 답변드리고, 그다음 제가 드리고 싶은 말 이어 적을게요ㅎㅎ

코드를 구현하면서 중복되는 코드들을 제거하려고 노력했지만 제가 발견하지 못한 코드들이 많은 거 같아요. 코드 중복을 줄이기 위해서 어떤 방법들을 고려하시는지 궁금합니다!

=> 하은님, 코드를 보면서 중복된 코드가 많다는 느낌을 받지 않았어요! 중복 코드 제거에 신경을 많이 쓰신 것 같아요ㅎㅎ 저도 코드 중복을 줄이기 위해 하은님처럼 컴포넌트로 추출하기도 하고, 비즈니스 로직의 경우에는 훅이나 모듈 파일로 분리하는 방법을 자주 사용해요ㅎㅎ 물론 중복된 코드를 줄이는 것은 중요하지만, 그 과정에서 가독성이 떨어지지 않도록 고민해보는 것도 중요한 것 같아요.

예를 들어, 컴포넌트를 너무 많이 추상화하거나 로직을 너무 많이 분리할 경우, 해당 로직을 파악하기 위해 여러 컴포넌트를 타고 들어가야 할 수도 있고, 기획 단위로 코드를 이해하기 어려울 수 있어요. 또한, 뒤에서 말씀드리겠지만, props drilling이 깊어질 가능성도 높아지구요ㅎㅎ 본인만의 적절한 균형을 찾는 것이 중요하다고 생각합니다!

ContentDisplay에 넘겨줄때 해당 데이터를 넘겨주어 본문 view를 보여주어야 할때 데이터를 보내주어야 하는데 저는 title,tag등의 전체 데이터를 넘겨주었어요. 아무래도 전달하기 편하고 바로 적용할 수 있기 때문에 이런식으로 넘겨주었는데 생각해보니 데이터가 자주 변경되거나 하면 문제가 발생할 수 있겠다라는 생각이 드네요.. 이럴때는 id만 넘겨주어 해당 id값을 다시 찾아 데이터를 불러오는 게 나을까요? 아니면 별 차이 없는지 궁금합니다!

=> 현재 postItem에 대한 수정 및 삭제 현황을 반영할 수 없어요. 예를 들어, ID로 상세 내용을 호출한다면 이미 삭제된 게시물의 경우 이를 보여주지 않고 목록을 강제로 새로고침해줄 수 있고, 수정된 내용을 보여줄 수 있을 것 같아요.

말씀하신 대로 데이터 변화 주기가 빠른 경우에는 사용자가 불편함을 느끼지 않는 선에서 데이터를 갱신해주는 것이 중요합니다. 이를 위해 상세 페이지에서 API 호출을 따로 하는 것이 좋을 것 같아요ㅎㅎ

반대로 데이터 변화 주기가 빠르지 않다면 API 호출 횟수를 줄여 비용을 절감할 수 있을 것 같아요! 어떤 방향이 프로젝트에 맞는지는 개발자가 항상 고민해야 하는 문제이고, 이러한 고민이 본인만의 정체성을 만든다고 생각합니다ㅎㅎ

한 컴포넌트에 너무 많은 코드를 집어넣지 않으려고 노력했습니다. 보통 한 컴포넌트에 몇 줄을 넣으시고 컴포넌트에 너무 많은 코드를 집어넣지 않으려는 노력 또는 기준이 있을까요?

=> 주관적인 방법임을 미리 말씀드릴게요ㅎㅎㅎ

저는 우선적으로 기획 단위로 코드를 작성하려고 노력해요! 기획 단위와 컴포넌트의 싱크를 맞춰놓으면 나중에 특정 기획에 대한 코드를 찾을 때 파악하기가 쉬웠어요ㅎㅎ

그런데 중복이 많이 일어나거나 특정 로직이 복잡해지거나, 에디터 14pt (제가 사용하는 폰트 사이즈입니다ㅎㅎ) 기준으로 한 페이지 내에 마크업과 비즈니스 로직이 모두 들어오지 않는 경우에는 컴포넌트를 추출하는 리팩토링을 진행해요ㅎㅎ


몇 가지 말씀드리고, 더불어 제가 권하는 액션을 말씀드릴게요ㅎㅎ

첫째로, 커밋 단위가 파일 별로 쪼개져 있어서 한 커밋 안에서 컴포넌트를 타고 들어갈 수가 없었어요. 요구사항 단위로 커밋을 쪼개보시면 관리하기도, 리뷰하기도 편해질 것 같아요ㅎㅎ

둘째로, handlePostClick이라는 함수가 PostItem에서만 호출됨에도 불구하고, App -> Main -> PostList -> PostItem 총 4개의 컴포넌트를 통해 props drilling 되고 있더라고요! 이 부분을 어떻게 개선할 수 있을지 고민해보시면 좋을 것 같습니다ㅎㅎ

짧지 않은 미션을 수행하시느라 수고 많으셨어요ㅎㅎ 리뷰어들도 고생 많았겠지만, 학교생활과 병행하면서 미션을 수행하는 것이 얼마나 많은 노력을 요구하는지 저도 잘 알기에 박수를 쳐드리고 싶습니다ㅎㅎ

리뷰를 하면서 정말 많은 고민을 하고 이 직무에 진심으로 임하고 계신다는 것을 느꼈어요ㅎㅎ 그리고 다들 잘하셔서 회사생활에 안주하고 있던 저에게도 한 줄기 자극이 되었습니다ㅎㅎ

현재 졸업이 1, 2년 남았을 수도 있고, 취준을 눈앞에 두고 있는 시점일 수도 있겠지만, 불안해하지 않으셨으면 좋겠습니다! 그저 열심히가 아닌 지금 하는 방향대로 열심히 하시면 충분히 좋은 결과가 있을 것이고, 제가 언제든 도와드릴 테니 부담 없이 자소서, 모의 면접, 코드 리뷰, 인생상담 등 요청 주세요ㅎㅎ

정말 고생 많으셨습니다!

ps. 늦게 리뷰드려서 죄송합니다...!

Comment on lines +57 to +69
const PostButton = styled.button`
color: black;
border: none;
padding: 0.75rem 1.5rem;
border-radius: 1.25rem;
cursor: pointer;
font-size: 1rem;
transition: all 0.3s ease;
display: flex;
align-items: center;
justify-content: center;
box-shadow: 0rem 0.25rem 0.5rem rgba(21, 19, 14, 0.2);
`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

css 컨벤션을 적용하는 연습도 해보면 좋을 것 같아요ㅎㅎ


const PostList = ({ postData, handlePostClick }) => {
const [currentPage, setCurrentPage] = useState(1);
const postsPerPage = 9;
Copy link
Collaborator

Choose a reason for hiding this comment

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

상수에 이름을 붙여주는 건 참 좋은 습관이에요ㅎㅎ

Comment on lines +13 to +14
<TitleWrapper>
<Title>Dount Blog</Title>
Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 딱 이코드만 봤을 때 TitleWrapper는 styled-component, Title은 일반 컴포넌트로 보이더라구요!
컴포넌트 네이밍은 의미가 명확하게 들어나는게 좋아요! 이름 짓는게 제일 어렵지만요ㅎㅎ
저는 styled-component에 <Styled라는 접두사를 붙이는걸 선호한답니다! 따라하실 필요는 없고, 아예 코드를 모르는 사람이 봤을 때 어떻게 구분할 수 있을까 고민해보시면 좋을 것 같아요ㅎㅎ

Click on the Vite and React logos to learn more
</p>
<Frame>
<Main
Copy link
Collaborator

Choose a reason for hiding this comment

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

App과 Main이 각각 무슨 역할인지 이름만 보고 파악하기가 힘들 수 있을 것 같아요ㅎㅎ
저라면 App -> Page로 변경하거나, Frame 안에 Header, PostList, PostDetail 이렇게 구성할 것 같아요!

<PageButton
key={pageNumber}
active={currentPage === pageNumber}
onClick={() => handlePageChange(pageNumber)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

setCurrentPage를 바로 사용하지 않고 함수로 뺀 이유가 있을까요??
과한 추상화는 오히려 가독성을 헤칠 수도 있을 것 같아요ㅎㅎ 각자의 코드스타일로 볼 수도 있지만, 다른 사람이 봤을 때 어떤 코드가 보기 편할지 고민해보시는건 중요해보입니다!

Comment on lines +18 to +20
setTitle("");
setTag("");
setBody("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

서로 연관이 있는 데이터로 보여서 각각의 데이터를 따로 관리하기보다 객체로 관리하면 어떨까요??

const [post, setPost] = useState({
                          title: '',
                          tag: '',
                          content: '',
                        });

Comment on lines +25 to +30
<Modal
isOpen={isOpen}
onRequestClose={onRequestClose}
className="CreatePostCss"
overlayClassName="overlay"
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

image 모달이 위와 같이 뜨는데, x 버튼이 input 영역 안에 있게 되면 해당 input 텍스트를 지운다는 쪽으로 인지하게 될 것 같네요ㅎㅎ x 버튼을 위로 따로 빼시면 더 좋을 것 같아요ㅎㅎ

))}
</PostFrame>
<PaginationWrapper>
{Array.from({ length: totalPages }, (_, i) => i + 1).map(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Array api를 잘 사용하시네요ㅎㅎ 깔끔해요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mission 미션 입니다!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants