TwoButtonModal을 기존의 Modal 컴포넌트로 대체 #721

inyeong-kang · 2023-10-06T09:56:44Z

🔥 연관 이슈

close: #686
close: #709

📝 작업 요약

TwoButtonModal 을 기존의 Modal 로 대체
마감시간 사용자지정 모달에서 초기화 버튼이 모달 밖으로 튀어나오는 UI 버그 해결

⏰ 소요 시간

1일 (반응형 같이 css 관련 삽질한 부분이 있었습니다..😂)

🔎 작업 상세 설명

기존 TwoButtonModal의 스타일링을 Modal에 흡수시켜 보았습니다~
또 TwoButtonModal의 props인 primaryButton, secondaryButton 도 Modal의 props로 추가해주었습니다.

기존 TwoButtonModal 모습
TwoButtonModal을 Modal 로 대체한 모습

🌟 논의 사항

모달의 종류와 쓰임새에 대해 잘 정리한 글이 있어 공유합니다!
https://brunch.co.kr/@tigrisdesign/3
Skyscanner가 Modal 에 대한 문서화가 굉장히 꼼꼼해서 공유합니다😀
https://backpack.github.io/storybook/?path=/story/bpk-component-modal-v2--multiple-modals

title, primaryButton, secondaryButton 추가 onModalClose 삭제

…로 수정

github-actions · 2023-10-06T09:58:06Z

⚡️ Lighthouse report!

Category	Score
🟠 Performance	66
🟠 Accessibilty	89
🟢 SEO	100
🟠 PWA	89

Category	Score
🟢 First Contentful Paint	0.6 s
🟠 Largest Contentful Paint	3.1 s
🔴 Total Blocking Time	1,780 ms
🟢 Cumulative Layout Shift	0
🟢 Speed Index	2.7 s

Gilpop8663

그리고 현재 모달은 div로 되어 있는데요 dialog 태그를 사용하면 웹 접근성을 좀 더 챙길 수 있을 것 같아요

dialog 태그를 사용하게 되면 탭 인덱스가 모달 내부에서만 동작하도록 되어서 불필요한 정보를 덜 주게 되어서 사용자가 헷갈리지 않을 것 같습니다

dialog로 만든 모달 예시) Drawer

-.VoTogether.-.Brave.2023-10-07.04-36-38.mp4

-.VoTogether.-.Brave.2023-10-07.04-40-07.mp4

현재 구현된 모달

-.VoTogether.-.Brave.2023-10-07.04-38-02.mp4

-.VoTogether.-.Brave.2023-10-07.04-40-19.mp4

현재 삭제 모달을 dialog로 변경해서 적용해본 예시

-.VoTogether.-.Brave.2023-10-07.05-30-33.mp4

dialog 태그로 변경하는 것은 제안이라서 가볍게 들으셔도 좋아요!!

또 다른 제안으로는 esc로 모달을 닫을 수 있으면 좋겠습니당
현재는 esc 이벤트는 없어서 아쉽다고 느꼈어요

참고자료

https://ui.toast.com/posts/ko_20220518

Gilpop8663 · 2023-10-06T19:18:12Z

frontend/src/components/common/DeleteModal/style.ts

 color: #67727e;

- font: var(--text-caption);
+ font-size: 14px;


--text-caption을 바꾸신 이유가 궁금해요

--text-caption: 400 1.6rem/2rem san-serif; 에서 font-size 가 원하는 크기보다는 살짝 커서 14px 로 수정했어요..!

Gilpop8663 · 2023-10-06T19:19:34Z

frontend/src/components/common/Modal/index.tsx

 useEffect(() => {
 const handler = (e: MouseEvent) => {
 if (e.target === BackDropRef.current) {
- onModalClose();
+ secondaryClick();


secondaryClick이 결국 닫기 이벤트를 의도하신 것 같은데 handleCloseClick 과 같이 모달이 닫힌다는 의미로 변수명을 짓는것은 어떠세요?

그리고 이 부분에서 secondaryClick()이 되는 로직으로 인하여 시간 사용자 지정을 할 때 배경을 누르면 초기화 하시겠습니까가 나와요

secondaryClick이 닫기 버튼이 아닐 때도 있어서 modalClose 함수를 따로 Props로 받는 것은 어떨지.. 고민이 되네요

-.VoTogether.-.Brave.2023-10-07.04-20-26.mp4

저도 버튼으로 어느 위치에 "닫기"가 올지 모른다고 생각이 들어서
props로 closeEvent를 내려받아서 esc, 배경클릭에 대한 이벤트를 대응하는 것이 어떨까 생각이 들어요~
아니면 두번째 버튼은 아예 닫기 버튼으로 고정시키는 게 좋을 것 같아요.

혹시 인지하지 못하고 다른 버튼 이벤트를 두번째로 내려줬다가 잘못 실행될 것 같아요.

기존에 있는 onModalClose를 부활시켰습니다😂
handleModalClose 로 닫기에 대해 대응할 수 있도록 해줬어요!

Gilpop8663 · 2023-10-06T19:24:32Z

frontend/src/components/common/Modal/style.ts

+ @media (max-width: ${theme.breakpoint.sm}) {
+ width: 96%;
+ }


이 부분은

(max-width: 576px) 까지일때는 width 96%로 보여줌

그 아래에 바로 min-width: 576px이 있어서 576px이 넘어가면 width: 50%로 보여줌

그래서 max-width 코드는 삭제하고 width:96%로 바로 주어도 동일할 것 같아요

제가 말한 것이 맞을까요??

넵 말씀하신 대로 의도한 것이 맞습니다. 잠깐 헷갈렸었나 봐요..ㅎㅎㅎ
아래처럼 수정했어요!

export const Container = styled.dialog<{ size: Size }>` display: flex; flex-direction: column; justify-content: space-between; position: fixed; top: 50%; left: 50%; transform: translate(-50%, -50%); width: ${props => (props.size ? MODAL_SIZE[props.size] : '96%')}; max-width: 290px; border-radius: 12px; border: 2px solid #f6f6f6; padding-left: 5px 0 0 10px; background-color: white; box-shadow: 0 0 6px 0 rgba(0, 0, 0, 0.5); @media (min-width: ${theme.breakpoint.sm}) { width: 50%; max-width: 500px; } `;

Gilpop8663 · 2023-10-06T19:25:27Z

frontend/src/components/common/TimePickerOptionList/style.ts

@@ -25,6 +25,6 @@ export const PickedTimeText = styled.p`
 align-items: center;
 gap: 20px;

- font: var(--text-caption);
+ font-size: 14px;


font-size를 바꾸신 이유가 궁금합니당 22

요거도 같은 이유입니다~ (--text-caption: 400 1.6rem/2rem san-serif; 에서 font-size 가 원하는 크기보다는 살짝 커서 14px 로 수정했어요..!)

Gilpop8663 · 2023-10-06T20:37:15Z

fe-리뷰완

chsua

모달이 여러 개였는데 가장 기본형 모달이 생겨서 좋네요!
다른 곳에서 모달을 사용할 일이 있다면 이걸 사용하면 될 것 같아요.
코드가 깔끔해지는 것을 보며 리팩토링이 중요하구나 느꼈습니다.

궁금한 점 등을 코멘트 달았습니다.
우스가 말씀하셨던 "두번째 버튼"이 취소버튼과 동일한 역할로 취급되는 것 같아서 그부분만 이야기를 나누어보면 좋을 것 같아요!

chsua · 2023-10-12T06:06:16Z

frontend/src/components/common/Accordion/Accordion.stories.tsx

 <ModalBody>
- <ModalTitle>정말 탈퇴하시겠어요?</ModalTitle>


훨씬 깔끔해졌어요!

chsua · 2023-10-12T06:08:39Z

frontend/src/components/common/DeleteModal/index.tsx

@@ -42,7 +42,7 @@ export default function DeleteModal({
 >
 <S.Description
 tabIndex={0}
- >{`${TARGET_FOR_DELETE[target]} 삭제하시겠습니까?\n${TARGET_FOR_DELETE[target]} 삭제하면 취소할 수 없습니다.`}</S.Description>
- </TwoButtonModal>
+ >{`${TARGET_FOR_DELETE[target]} 삭제하시겠습니까?\n삭제 버튼 클릭 시 취소할 수 없습니다.`}</S.Description>


혹시 이렇게 되면 계정 삭제하시겠습니까? 이렇게 나오나요?

넵 맞습니다~

chsua · 2023-10-12T06:15:36Z

frontend/src/components/common/Modal/index.tsx

 useEffect(() => {
 const handler = (e: MouseEvent) => {
 if (e.target === BackDropRef.current) {
- onModalClose();
+ secondaryClick();


저도 버튼으로 어느 위치에 "닫기"가 올지 모른다고 생각이 들어서
props로 closeEvent를 내려받아서 esc, 배경클릭에 대한 이벤트를 대응하는 것이 어떨까 생각이 들어요~
아니면 두번째 버튼은 아예 닫기 버튼으로 고정시키는 게 좋을 것 같아요.

혹시 인지하지 못하고 다른 버튼 이벤트를 두번째로 내려줬다가 잘못 실행될 것 같아요.

chsua · 2023-10-12T06:18:09Z

frontend/src/components/common/Modal/index.tsx

+ <S.Title aria-label={`제목: ${title}`} tabIndex={0}>
+ {title}
+ </S.Title>
+ <S.Body>{children}</S.Body>


크게 상관 없는 부분이지만 children이 없는 경우가 있는데
{ children && <S.Body>{children}</S.Body> }
이렇게 처리하지 않으신 이유가 있나요?
min-hight가 100%로 들어가있던데 관련이 있나요?

음 생각해보니 PropsWithChildren이라 children을 안넣는 경우도 있겠네요!
말씀해주신 부분 반영하겠습니다😄

min-height도 삭제해줬습니다! (Modal에 내용 없이 버튼만 넣고 싶은 경우도 고려해야 하므로...)

chsua · 2023-10-12T06:26:42Z

frontend/src/components/common/Modal/style.ts

+
+ @media (min-width: ${theme.breakpoint.sm}) {
+ width: 50%;
+ max-width: 500px;


가로길이가 많이 설정되어서 어떻게 되는건지 잘 모르겠어요😂

ㅎㅎㅎ인정합니다...(저도 코드 쓰면서 헷갈렸어요😂) 우스도 이 부분 코멘트 주셔서 수정했어요!

chsua · 2023-10-12T06:30:22Z

fe-리뷰완

inyeong-kang · 2023-10-18T06:51:20Z

Modal의 props로 handleModalClose 추가 (여기에 모달 닫기 관련 함수 할당)
Modal의 width 관련 스타일 수정

fe-리뷰완

Gilpop8663

코드 확인 후 어프로브 했습니다

최고!

fe-리뷰완

Gilpop8663 · 2023-10-18T20:42:35Z

frontend/src/components/common/Modal/style.ts

+ margin-top: 5px;
+ margin-bottom: 16px;


이 부분도 아래와 같이 한줄로 사용할 수 있겠어요

Gilpop8663 · 2023-10-18T20:49:29Z

frontend/src/components/common/Modal/style.ts

 border-radius: 12px;
 border: 2px solid #f6f6f6;
- padding: 5px;
+ padding-left: 5px 0 0 10px;


혹시 padding-left가 아니라 padding 아닌가요?

오.... 매의 눈 감사합니다 ㅎㅎ

그런데 해당 속성은 필요하지 않아져서 삭제했어요~~!

Gilpop8663 · 2023-10-18T20:52:15Z

frontend/src/components/common/Modal/index.tsx


 return (
 <S.All>
 <S.HiddenCloseButton
- onClick={onModalClose}
+ onClick={handleModalClose}
 tabIndex={0}
 aria-label="팝업 창 닫기"
 ></S.HiddenCloseButton>
 <S.Backdrop ref={BackDropRef}></S.Backdrop>
 <S.Container tabIndex={0} size={size}>


dialog로 바뀌었는데 open이라는 속성 없이도 잘 보이네요! 신기한데용

Gilpop8663 · 2023-10-18T20:56:14Z

fe-리뷰완

inyeong-kang · 2023-10-19T02:38:07Z

우스 코멘트 반영했습니다!

fe-리뷰요청

chsua

수정된 코드 읽었습니다!
고생하셨습니다!!
최고에요!

inyeong-kang added 4 commits October 6, 2023 13:50

refactor: (#686) Modal 컴포넌트 props 수정

a3237c3

title, primaryButton, secondaryButton 추가 onModalClose 삭제

refactor: (#686) TwoButtonModal을 Modal 컴포넌트로 대체

4e366b7

design: (#686) Modal 스타일링 수정, size를 optional props로 수정

7d8ecde

design: (#686) DeleteModal, ReportModal 스타일링 수정, size를 optional props…

647eb7a

…로 수정

inyeong-kang requested review from chsua and Gilpop8663 October 6, 2023 09:56

inyeong-kang self-assigned this Oct 6, 2023

inyeong-kang changed the title ~~Refactor/#686~~ TwoButtonModal 을 기존의 Modal 컴포넌트로 대체 Oct 6, 2023

inyeong-kang changed the title ~~TwoButtonModal 을 기존의 Modal 컴포넌트로 대체~~ TwoButtonModal을 기존의 Modal 컴포넌트로 대체 Oct 6, 2023

inyeong-kang added FE 🛠 리팩터링 🐛 버그 labels Oct 6, 2023

Gilpop8663 requested changes Oct 6, 2023

View reviewed changes

chsua reviewed Oct 12, 2023

View reviewed changes

inyeong-kang added 6 commits October 18, 2023 14:55

Merge branch 'dev' into refactor/#686

6e5e413

refactor: (#686) 모달의 div 태그를 dialog 태그로 변경

8c12936

refactor: (#686) Modal에 handleModalClose props 추가

72e69dc

refactor: (#686) Modal의 children 이 없는 경우 반영

3edbc94

chore: (#686) 충돌 해결 중 삭제된 코드 복구, size가 있는 경우 width 값을 size로 고정

cdb44d7

design: (#686) 회원 탈퇴 모달 스타일링 수정

1d6e46a

Gilpop8663 approved these changes Oct 18, 2023

View reviewed changes

design: (#686) margin 인라인으로 작성, padding 제거

00c2dae

chsua approved these changes Oct 19, 2023

View reviewed changes

chsua merged commit abf6674 into dev Oct 19, 2023
1 check passed

woo-chang deleted the refactor/#686 branch October 19, 2023 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TwoButtonModal을 기존의 Modal 컴포넌트로 대체 #721

TwoButtonModal을 기존의 Modal 컴포넌트로 대체 #721

inyeong-kang commented Oct 6, 2023 •

edited

Loading

github-actions bot commented Oct 6, 2023 •

edited

Loading

Gilpop8663 left a comment •

edited

Loading

Gilpop8663 Oct 6, 2023

inyeong-kang Oct 18, 2023

Gilpop8663 Oct 6, 2023

Gilpop8663 Oct 6, 2023

chsua Oct 12, 2023

inyeong-kang Oct 18, 2023

Gilpop8663 Oct 6, 2023

inyeong-kang Oct 18, 2023

Gilpop8663 Oct 6, 2023

inyeong-kang Oct 18, 2023

Gilpop8663 commented Oct 6, 2023

chsua left a comment

chsua Oct 12, 2023

chsua Oct 12, 2023

inyeong-kang Oct 18, 2023

chsua Oct 12, 2023

chsua Oct 12, 2023

inyeong-kang Oct 18, 2023

inyeong-kang Oct 18, 2023

chsua Oct 12, 2023

inyeong-kang Oct 18, 2023

chsua commented Oct 12, 2023

inyeong-kang commented Oct 18, 2023

Gilpop8663 left a comment

Gilpop8663 Oct 18, 2023

Gilpop8663 Oct 18, 2023

inyeong-kang Oct 19, 2023

inyeong-kang Oct 19, 2023

Gilpop8663 Oct 18, 2023

Gilpop8663 commented Oct 18, 2023

inyeong-kang commented Oct 19, 2023

chsua left a comment

		<ModalBody>
		<ModalTitle>정말 탈퇴하시겠어요?</ModalTitle>

TwoButtonModal을 기존의 Modal 컴포넌트로 대체 #721

TwoButtonModal을 기존의 Modal 컴포넌트로 대체 #721

Conversation

inyeong-kang commented Oct 6, 2023 • edited Loading

🔥 연관 이슈

📝 작업 요약

⏰ 소요 시간

🔎 작업 상세 설명

🌟 논의 사항

github-actions bot commented Oct 6, 2023 • edited Loading

Gilpop8663 left a comment • edited Loading

Choose a reason for hiding this comment

참고자료

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gilpop8663 commented Oct 6, 2023

chsua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsua commented Oct 12, 2023

inyeong-kang commented Oct 18, 2023

Gilpop8663 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gilpop8663 commented Oct 18, 2023

inyeong-kang commented Oct 19, 2023

chsua left a comment

Choose a reason for hiding this comment

inyeong-kang commented Oct 6, 2023 •

edited

Loading

github-actions bot commented Oct 6, 2023 •

edited

Loading

Gilpop8663 left a comment •

edited

Loading