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

[NDD-148] modal 컴포넌트 구현하기 (4h/3h) #37

Merged
merged 9 commits into from
Nov 14, 2023
Merged

Conversation

Yoon-Hae-Min
Copy link
Collaborator

@Yoon-Hae-Min Yoon-Hae-Min commented Nov 13, 2023

NDD-148 Powered by Pull Request Badge

Why

modal 컴포넌트를 compound component 패턴으로 구현하며 상황에 맞게 컴포넌트를 조합하도록 설정하였습니다.
구현자체는 2시간에 완성 했지만 close button에 layout에 주입하는 매서드를 넣고 싶어서 해당 로직을 작성하고 리펙토링하는데 시간을 좀 쓴것 같습니다.

How

image
다음과 같은 형태로 사용할 수 있습니다. 구성요소인 header, footer는 필요시 넣을 수 있고 만약 기본 modal이라면 content만 넣어서 직접 modal을 디자인 할 수 있도록 설정했습니다.
아마 해당 modal을 바탕으로 확장해서 작업하면 될것 같습니다.

포인트 1

디자인 쪽에서 closebutton이 있었는데요
image
현재 같은 구조에서는 close버튼을 클릭시에 modal을 종료 하려면 modal.header에도 closeModal매서드를 넣어주어야 하고 modal에서도 closeModal매서드를 주입해주어야 하는 문제점이 생깁니다. 이에 closeModal이라는 매서드는 가장 상위 element인 modal에서 관리를 해주는것이 좋다 생각 하였고 그에 따라 utils를 생성하며 자식 노드에 해당 props를 추가하는 enhanceChildElement를 만들었습니다.

포인트2

theme의 그림자 영역을 보니 컬러값이랑 같이 관리가 되고 있더라고요? 그래서 색상을 분리하였고 modal 그림자 색상을 임의로 추가하였습니다.

Result

modal의 구성요소가 전부 다 있을 시
image

기본 content만 있을시
image

Reference

utils의 타입을 맞추기 위해서 참고한 글
https://medium.com/@cristiansima/typescript-ing-react-cloneelement-or-how-to-type-a-child-element-with-props-injected-by-the-parent-73b6ad485f8b

@Yoon-Hae-Min Yoon-Hae-Min added FE 프론트엔드 코드 변경사항 feature 새로운 기능이 추가 된 경우 labels Nov 13, 2023
@Yoon-Hae-Min Yoon-Hae-Min self-assigned this Nov 13, 2023
Copy link
Collaborator

@milk717 milk717 left a comment

Choose a reason for hiding this comment

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

확장성도 좋은 아주 이쁜 Modal 코드가 완성되었군요!!
고생하셨습니다!!

@@ -0,0 +1,16 @@
import { css } from '@emotion/react';
import { PropsWithChildren } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-5] 이런 타입을 제공해주는군요!! 하나 배워갑니다~

Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-5] 아하 이게 도입되면 여러가지로 바꿔야 할 컴포넌트들이 있을거 같은데요 (Layout 이라던가...) 하지만 이건 나중에 처리하도록 하죠!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adultlee 엇 요거는 그냥 React내부에서 children의 타입만 명시하는 타입이여서 따로 변경되어야 하는 점이 없다고 생각하는데 혹시 어떤 부분일까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

다른 컴포넌트(layout)에서는 children을 따로 type을 명시해서 받습니다!
image

요렇게 말이죠! 아마 한번에 바꿔줄 시점이 필요할듯 합니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아아 이해했습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-4] index.tsx에서 어디까지 담당할지와 이 컴포넌트처럼 한 세트로 묶이는 컴포넌트를 어떻게 정의할지 한번 이야기해보면 좋겠습니다. 참고로 저는 Accordian 컴포넌트를 만들 때 (e904a99) index.tsx에서는 export 해서 사용했습니다.
그래서 사용할 때 아래와 같은 형식으로 사용했는데요. 해민님의 방법으로 하면 Accordion, Accordion.Summary, Accordion.Details로 변경되겠네요!
한 묶음으로 사용하는 것이 명확하게 명시돼서 좋을 것 같은데 저는 개인적으로 자동완성이 더 편리해서 Accordian컴포넌트에서 사용했던 방식을 선호하긴 합니다만 이 부분은 취향 문제에 가까우니 다음번 만남 때 FE끼리 가볍게 이야기해봅시다!!

<Accordion>
  <AccordionSummary>
    펼치지 않아도 보이는 부분
  </AccordionSummary>
  <AccordionDetails>
    펼치면 보이는 부분
  </AccordionDetails>
</Accordion>

Copy link
Collaborator

Choose a reason for hiding this comment

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

흠 아무래도 해민님 방식이 압도적으로 이쁘구 좋을거 같긴합니다! 아마 확장성도 좋지 않을까 싶네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

뭔가 해당 컴포넌트에 대해서 강하게 묶여있으면 제가 짠 코드처럼 사용하고 아니라면 수민님이 사용하는 코드처럼 짤것 같아요 예를 들면 제 modal.content는 modal내부에서만 사용가능하게 하고 싶은거고 수민님이 만들어주신 AccordionSummary는 단독으로도 사용이 가능할 수 있다는 그런 의도가 있다고 생각합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-5] 아주 굿입니다!!👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

아주 훌륭하군요 이견없습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-5] 너무 좋은데요!! 나중에 Accordion 컴포넌트도 이 유틸함수 사용해서 수정해볼게요~

Copy link
Collaborator Author

@Yoon-Hae-Min Yoon-Hae-Min Nov 13, 2023

Choose a reason for hiding this comment

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

오늘의 하이라이트 유틸입니다 타입이 좀 힘들었습니다. 솔직히 아직도 뭔가 타이핑을 잘못한거 같은 느낌이 듭니다...

return (
<div
css={css`
display: flex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-1]

Suggested change
display: flex;
display: flex;
align-items: center;

해민님이 PR 본문에 올려주신 모달에서는 괜찮은데 제 컴에서 실행할 때는 가운데 정렬이 약간 안맞아서 X 버튼이 위로 올라갑니다! align-items: center; 추가 부탁드립니다~

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

일단 align-items: center;를 붙이게 되면 header가 두줄일 시에 오른쪽 상단에 붙는게 아니라 2줄 중앙에 붙이는 형태로 동작이 됩니다

그래서 조금 더 고민해서 다음과 같이 적용했습니다. 한번 확인해 주세요
32c422c

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

arc 브라우저 시크릿 모드로 실행했을 때는 수정된 버전도 약간 위로가네요...
그런데 chrome 브라우저에서는 가운데로 갑니다!! 해민님은 arc 브라우저에서 실행하면 어떻게 보이시나요??

padding: 1rem;
color: ${theme.colors.text.white};
background-color: ${theme.colors.point.secondary.default};
border-radius: 1rem 1rem 0rem 0rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-3] 모달의 위쪽 border 부분에 얇게 흰 테두리가 보이네요. border-radius는 모두 1rem으로 맞춰져있는데 왜 저 테두리가 보이는걸까요...??
원인은 찾지 못했지만 MVP 배포 전에만 거슬리지 않게 처리하면 좋을 것 같습니다!

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

매의눈이시네요! 저도 살짝 거슬렸습니다!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

parent element인 layout에서 overflow hidden을 주고 header의 border-radius를 제외해도 똑같이 발생하는 것인걸 토대로 이건 브라우저가 paint할때 무슨 버그가 있다고 추축은 합니다.
제 검색으로는 왜 일어나는지는 모르고 다들 해결 방법만 써있네요 ㅠㅠ 찾은 방법인 margin -1px을 둬서 해결은 했습니다.
17f0f79

Copy link
Collaborator

Choose a reason for hiding this comment

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

아하 margin -1px은 다소 이상하긴 하네요 ㅠㅠ 다른 방법이 없다면 우선 진행하시죠!

Copy link
Collaborator

Choose a reason for hiding this comment

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

차라리 Header에 border를 따로 주는건 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

적용해 봤는데 똑같습니다 ㅠㅠ 제 추축으로는 border를 그릴때 문제인지라 border를 제외한 다른 방법을 사용해야 하지 않을까 추축합니다.

저도 마음에 들지는 않지만 가장 유명하고 간단한 방법이 maring -1px였네요 ㅠㅠ

@@ -5,7 +5,7 @@ import React from 'react';

type BoxProps = HTMLElementTypes<HTMLDivElement>;

const Box: React.FC<BoxProps> = ({ children }, ...args) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-5] 와우... 이걸 이제야 보다니😂
무표적으로 작업하다가 이거보고 빵 터졌어요! 수정되서 다행입니다!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

아주 훌륭하군요 이견없습니다!

@@ -0,0 +1,16 @@
import { css } from '@emotion/react';
import { PropsWithChildren } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

[p-5] 아하 이게 도입되면 여러가지로 바꿔야 할 컴포넌트들이 있을거 같은데요 (Layout 이라던가...) 하지만 이건 나중에 처리하도록 하죠!

padding: 1rem;
color: ${theme.colors.text.white};
background-color: ${theme.colors.point.secondary.default};
border-radius: 1rem 1rem 0rem 0rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

매의눈이시네요! 저도 살짝 거슬렸습니다!!

>
<Box
css={css`
z-index: 101;
Copy link
Collaborator

Choose a reason for hiding this comment

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

[ p-5] z-index에 대한 규칙도 명시되면 좋을듯 합니다!! 아마도 이건 따로 명시가 되어야 겠지요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

z-index도 theme로 관리하면 어떨까 싶긴하네요?!
이건 의논이 필요할듯 합니다.
이유) 아직은 아니지만 header 를 공통적으로 사용하는 페이지가 생겨나고 이를 z-index로 관리해서 스크롤에 대응할때 규칙화 되길 원함

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저는 처음보는 방식이라 어떻게 관리가 되는지 궁금하네요?!?

Copy link
Collaborator

Choose a reason for hiding this comment

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

image https://oddcode.tistory.com/147 이런 내용이었습니다!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

흠 아무래도 해민님 방식이 압도적으로 이쁘구 좋을거 같긴합니다! 아마 확장성도 좋지 않을까 싶네요!

Copy link

cloudflare-pages bot commented Nov 13, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a0a07f7
Status: ✅  Deploy successful!
Preview URL: https://c1ad29ef.gomterview.pages.dev
Branch Preview URL: https://feature-ndd-148.gomterview.pages.dev

View logs

@Yoon-Hae-Min Yoon-Hae-Min merged commit ab04855 into dev Nov 14, 2023
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the feature/NDD-148 branch November 14, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 프론트엔드 코드 변경사항 feature 새로운 기능이 추가 된 경우
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants