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

Feat/#550 Confirm Modal 구현 #553

Merged
merged 23 commits into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d3aee9e
feat: confirm provider 뼈대 구현
ukkodeveloper Nov 3, 2023
3a8246e
feat: confirm modal 수락 기능 추가
ukkodeveloper Nov 3, 2023
31ecdd0
feat: confirmModal 스타일 및 분리
ukkodeveloper Nov 4, 2023
580124d
feat: ConfirmProvider 분리
ukkodeveloper Nov 4, 2023
3273fa7
feat: ConfirmModal storybook 추가
ukkodeveloper Nov 4, 2023
c25781d
feat: storybook 예시 추가
ukkodeveloper Nov 5, 2023
d526aa9
refactor: useConfirm 분리
ukkodeveloper Nov 5, 2023
e382050
refactor: confirm modal 로직 응집하기 위해 useEffect 이동
ukkodeveloper Nov 5, 2023
a526142
fix: promise에서 resolve 함수 상태 변경 실패로 인한 ref 사용
ukkodeveloper Nov 5, 2023
1179f35
refactor: createPortal를 ConfirmModal 내부로 이동
ukkodeveloper Nov 5, 2023
dbc1c32
refactor: 의미있는 네이밍으로 변경
ukkodeveloper Nov 5, 2023
2af5e0b
fix: keydown 이벤트 적용되지 않는 현상 수정
ukkodeveloper Nov 6, 2023
6441a12
style: style lint 적용 및 개행
ukkodeveloper Nov 6, 2023
3f60df5
chore: 사용하지 않는 파일 삭제
ukkodeveloper Nov 14, 2023
a225ac0
fix: resolverRef 타입 변경
ukkodeveloper Nov 14, 2023
aab67ba
feat: 닫기, 수락 button에 type 추가
ukkodeveloper Nov 14, 2023
67963b6
refactor: 네이밍 cancel에서 denial으로 변경
ukkodeveloper Nov 14, 2023
b1ad8a7
feat: 모달 열릴 때 바로 title로 포커스 이동할 수 있도록 수정
ukkodeveloper Nov 14, 2023
ac5d64c
refactor: 중복된 createPortal 삭제
ukkodeveloper Nov 16, 2023
f27bd28
refactor: theme 활용하여 색상 코드 변경
ukkodeveloper Nov 17, 2023
a6eb162
refactor: 전반적인 ConfirmModal 네이밍 변경
ukkodeveloper Nov 17, 2023
a2fb196
fix: theme color 사용 시 객체분해할당 오류 수정
ukkodeveloper Nov 17, 2023
b94ea94
Merge branch 'main' into feat/#550
ukkodeveloper Nov 23, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ const RegisterButton = styled.button`

@media (min-width: ${({ theme }) => theme.breakPoints.md}) {
padding: 11px 15px;

font-size: 18px;
}
`;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import styled from 'styled-components';
import ConfirmProvider from './ConfirmModalProvider';
import { useConfirm } from './hooks/useConfirm';
import type { Meta, StoryObj } from '@storybook/react';

const meta: Meta<typeof ConfirmProvider> = {
title: 'shared/Confirm',
component: ConfirmProvider,
decorators: [
(Story) => (
<ConfirmProvider>
<Story />
</ConfirmProvider>
),
],
};

export default meta;

type Story = StoryObj<typeof ConfirmProvider>;

export const Example: Story = {
render: () => {
const Modal = () => {
const { confirm } = useConfirm();

const clickHiByeBtn = async () => {
const isConfirmed = await confirm({
title: '하이바이 모달',
content: (
<>
<p>도밥은 정말 도밥입니까?</p>
<p>코난은 정말 코난입니까?</p>
</>
),
denial: '바이',
confirmation: '하이',
});

if (isConfirmed) {
alert('confirmed');
return;
}

alert('denied');
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 저는 처음에 모달을 보고 사용처에서 onDeny, onConfirmed 등의 props로 실행할 함수를 전달해서 사용할 수 있을 거라 예상했어요.

예상과는 다르게, 스토리북코드에서 처럼 confirm함수의 결과물로 나온 boolean 값으로 명령형 로직을 작성해야 하더라고요.

onDeny, onConfirmed는 따로 조작해야 하도록 하신 이유가 있을까요?
우코의 컴포넌트 작성의도가 궁금해요~ 설명을 해주시면 좋겠습니당~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

도밥 말처럼 onDeny, onConfirmed로 함수를 인자로 넣을 수 있게 만들어도 됩니당!
하지만 모든 것을 props로 받는것 보다
핸들러 로직에서 confirm이 되었을 때 ~~를 하겠다고 명령형으로 적는 게 코드 읽는 데 더 편하지 않을까요?

https://developer.mozilla.org/en-US/docs/Web/API/Window/confirm

Copy link
Collaborator

Choose a reason for hiding this comment

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

       const isConfirmed = await confirm({
          title: '오쁜클로즈 모달',
          content: (
            <>
              <p>코난은 정말 코난입니까?</p>
              <p>도밥은 정말 도밥입니까?</p>
            </>
          ),
        });

        if (isConfirmed) {
          alert('confirmed');
          return;
        }

        alert('denied');

저는 명령형 로직을 작성하지 않아도 된다는게 react의 장점이라고 생각해요.

const doA = () => {}

const doB = () => {}

return <ConfirmModal onDeny={doA} onConfirm={doB}></ConfirmModal>

처음에는 우코가 작성한 인터페이스가 낯설고 익숙하지 않아서
그 의도를 이해하지 못했는데요~! 지금도 우코의 생각을 모두 이해한것 같지는 않지만, 컨펌 모달을 전역으로 사용할 수 있다는 규제 완화? 와 함께 합리적인 인터페이스를 구현하셨다고 생각이 들어요.
사용해보고 나중에 또 이야기해 보고싶네요 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

const doA = () => {}

const doB = () => {}


// 많은 코드...

return <ConfirmModal onDeny={doA} onConfirm={doB}></ConfirmModal>

이 사이에 많은 코드들이 추가된다면 어떻게 될까요?같은 역할을 하는 코드가 흩어져서 위아래로 번갈아가면서 읽어야할 것 같아요!

제가 만든 훅은 JSX(UI)와 로직을 한 데 뭉쳐두는 것이 목적이었습니다. UI 부분은 선언적으로 처리하고, 수락/거부했을 때 로직은 명령형으로 작성했습니다.

저는 선언적이라고 반드시 좋고, 명령형이라고 해서 반드시 나쁘다고 생각하지 않아요.

만약 책의 표지에서 제목(선언적 표현)만으로 독자에게 어떤 책인지 표현할 수 없다면, 부제나 요약설명(절차적 표현)을 밖으로 드러내는 것과 같다고 봅니다. 책 겉표지에서 제대로 이해가 안되면 독자는 내부를 읽어볼 수밖에 없으니까요.


// denial과 confirmation 기본값은 '닫기'와 '확인'입니다.
const clickOpenCloseBtn = async () => {
const isConfirmed = await confirm({
title: '오쁜클로즈 모달',
content: (
<>
<p>코난은 정말 코난입니까?</p>
<p>도밥은 정말 도밥입니까?</p>
</>
),
});

if (isConfirmed) {
alert('confirmed');
return;
}

alert('denied');
};

return (
<Body>
<Button onClick={clickHiByeBtn}>하이바이 모달열기</Button>
<Button onClick={clickOpenCloseBtn}>닫기확인 모달열기</Button>
</Body>
);
};

return <Modal />;
},
};

const Body = styled.div`
height: 2400px;
`;

const Button = styled.button`
padding: 4px 11px;
color: white;
border: 2px solid white;
border-radius: 4px;
`;
106 changes: 106 additions & 0 deletions frontend/src/shared/components/ConfirmModal/ConfirmModal.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import { createPortal } from 'react-dom';
import { Flex } from 'shook-layout';
import styled, { css } from 'styled-components';
import Spacing from '../Spacing';
import type { ReactNode } from 'react';

interface ConfirmModalProps {
title: string;
content: ReactNode;
denial: string;
confirmation: string;
onDeny: () => void;
onConfirm: () => void;
}

const ConfirmModal = ({
title,
content,
denial,
confirmation,
onDeny,
onConfirm,
}: ConfirmModalProps) => {
return createPortal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

포탈이 모달에 1번, provider에 1번 총 2번 적용되어있어요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

수정 완료했습니다!

<>
<Backdrop role="dialog" aria-modal="true" />
<Container>
<Title>{title}</Title>
<Spacing direction="vertical" size={10} />
<Content>{content}</Content>
<Spacing direction="vertical" size={10} />
<ButtonFlex $gap={16}>
<CancelButton onClick={onDeny}>{denial}</CancelButton>
<ConfirmButton onClick={onConfirm}>{confirmation}</ConfirmButton>
</ButtonFlex>
</Container>
</>,
document.body
);
};

export default ConfirmModal;

const Backdrop = styled.div`
position: fixed;
top: 0;
right: 0;
bottom: 0;
left: 0;
Comment on lines +54 to +57
Copy link
Collaborator

Choose a reason for hiding this comment

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

inset 으로 1줄로 줄여볼까용~?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋아요!


width: 100%;
height: 100%;
margin: 0;
padding: 0;

background-color: rgba(0, 0, 0, 0.7);
`;

const Container = styled.section`
position: fixed;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);

min-width: 300px;
margin: 0 auto;
padding: 24px;

color: #ffffff;
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 theme .color.white 레츠고우~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이것도 함께 이야기 나누면 좋을 것 같아용. 항상 color 값 줄때마다 ${({theme}) => theme.color.white}를 적는 게 불편한데 도밥은 어떠신가요? 저는 차라리 객체로 관리하는 게 낫지 않을까 생각도 듭니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

공감합니다. 테마를 동적으로 바꾸는 일도 없어서 더욱더 그렇네용
다만 일단은 컨벤션에 맞게 작성해두고, 한번에 바꾸는게 좋을 것 같아요

저도 현 구조에서 theme의 장점이 있을지 한번 더 생각해볼게요!
(한번 생각해봤는데 우선은 없네요 ㅋㅋㅋ...)


background-color: #17171c;
border: none;
border-radius: 16px;
`;

const ButtonFlex = styled(Flex)`
width: 100%;
`;

const Title = styled.header`
font-size: 18px;
text-align: left;
`;

const Content = styled.div``;

const buttonStyle = css`
flex: 1;

width: 100%;
height: 36px;

color: ${({ theme: { color } }) => color.white};

border-radius: 10px;
`;

const CancelButton = styled.button`
background-color: ${({ theme: { color } }) => color.secondary};
${buttonStyle}
`;

const ConfirmButton = styled.button`
background-color: ${({ theme: { color } }) => color.primary};
${buttonStyle}
`;
112 changes: 112 additions & 0 deletions frontend/src/shared/components/ConfirmModal/ConfirmModalProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/* eslint-disable react-hooks/exhaustive-deps */
import { createContext, useCallback, useEffect, useState, useRef } from 'react';
import { createPortal } from 'react-dom';
import ConfirmModal from './ConfirmModal';
import type { ReactNode } from 'react';

export const ConfirmContext = createContext<null | {
confirm: (modalState: ModalContents) => Promise<boolean>;
}>(null);

interface ModalContents {
title: string;
content: ReactNode;
denial?: string;
confirmation?: string;
}
Comment on lines +11 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 denial, confirmation 메세지가 옵셔널이네요~
혹시 실제 Modal에 기본값과 옵셔널 처리를 하지않고 여기서 한 이유가 있을까요~?

Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 요거 위쪽의 ConfirmModalProps랑 비슷한 타입이라서 재사용을 해볼 수도 있을것 같기도 하고요~ 의도를 몰라서 우선 남겨봅니다~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

denial, confirmation 메세지가 옵셔널이네요~
이 부분은 버튼 이름입니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

요거 위쪽의 ConfirmModalProps랑 비슷한 타입이라서 재사용을 해볼 수도 있을것 같기도 하고요~ 의도를 몰라서 우선 남겨봅니다~

제가 조심스럽게 타입을 사용하는 경향이 있어요! 우선 ConfirmModalProps 는 이제 modal 을 사용하는 곳에서 사용할 interface이고, ModalContents 는 내부에서 관리하고 있는 상태에요! 지금은 비슷해 보이면서도 성격이 다른 것 같아서 분리해 놓았습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분은 버튼 이름입니다!

아 네엡 저도 버튼의 메세지를 말한거였습니당

실제 Modal에 기본값과 옵셔널 처리를 하지않고 여기서 한 이유가 있을까요~?

이게 메인 질문이었어요!


const ConfirmProvider = ({ children }: { children: ReactNode }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 저는 children이 옵셔널한게 추후에 해당 provider의 쓰임이 변경될때 조금 더 유연하게 대처할 수 있다고 생각해요

우코는 어떻게 생각하나요~?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Provider에 children을 optional하게 두면, children을 넣지 않을 수도 있다는 뜻인가요? 그러면 context api를 어느 컴포넌트에서도 사용하지 못하게 될 것 같은데, "조금 더 유연하게 대처"할 수 있다는 도밥의 의견을 더 자세하게 설명해주시면 고려해볼게요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

그러면 context api를 어느 컴포넌트에서도 사용하지 못하게 될 것 같은데,

이 부분을 조금 다르게 말해서, chidren을 사용하지 않게 되는 경우에도 열려있게 하면 어떨까 했어요. 컴포넌트 구현이 수정될 수도 있으니까요~

예를들면 저번에 children: ReactNode[ ] 로 지정되었던 Context처럼 당시에는 '절대 다르게 쓰일일이 없다~' 라고 생각하고 type을 좁여 놓았다가,
나중에 ReactNode로 사용하고 싶을때 컴포넌트를 수정해야 했던것 처럼요.

하지만 우코의 말대로 옵셔널하게 열지 않고 좁혀놓아도 좋다고 생각합니다~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

제가 그때는 타입을 잘못 이해하고 있어서 사용했던 거였어요!

  type ReactNode =
        | ReactElement
        | string
        | number
        | Iterable<ReactNode>
        | ReactPortal
        | boolean
        | null
        | undefined

ReactNode 타입에 기본적으로 Iterable<ReactNode>이 있더라고요! 그래서 그때는 실수였습니다.


그거와는 별개로 현재상황에서는 ReactNode를 Optional이면 안된다고 생각해요. Provider을 사용한다는 것이 하위 React Element에서 Context를 주입할 수 있다는 점이니까요. 예상되는 문제점, 혹은 예상되지 않더라도 혹여나 문제가 될거라 짐작되는 부분이 있으면 언제든 말씀해주세요!

const [isOpen, setIsOpen] = useState(false);
const resolverRef = useRef<{
resolve: (value: boolean | PromiseLike<boolean>) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 PromiseLike 타입은 왜 있는건가요?

} | null>(null);
const [modalContents, setModalContents] = useState<ModalContents>({
title: '',
content: '',
denial: '닫기',
confirmation: '확인',
});
const { title, content, denial, confirmation } = modalContents;

// ContextAPI를 통해 confirm 함수만 제공합니다.
const confirm = (contents: ModalContents) => {
openModal();
setModalContents(contents);

const promise = new Promise<boolean>((resolve) => {
resolverRef.current = { resolve };
});

return promise;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 이 함수를 컨펌 모달 사용처에서 사용하면 되는 것 같은데요~!
내부적으로 promise가 필요한 이유는 무엇인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

confirm을 하기 전 까지는 다른 작업을 막을 수 있습니다.


const closeModal = () => {
setIsOpen(false);
};

const openModal = () => {
setIsOpen(true);
};

const resolveConfirmation = (status: boolean) => {
if (resolverRef?.current) {
resolverRef.current.resolve(status);
}
};

const onDeny = useCallback(() => {
resolveConfirmation(false);
closeModal();
}, []);

const onConfirm = useCallback(() => {
resolveConfirmation(true);
closeModal();
}, []);

const onKeyDown = useCallback((event: KeyboardEvent) => {
const { key } = event;
if (key === 'Enter') {
event.preventDefault();
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.

포커스가 trigger button에 있어서 엔터 누르면 트리거가 실행됩니당. focus를 강제 이동시키는 방법도 있는데, 텍스트(타이틀)에 포커스를 두게 하는 게 좋은 것인지는 찾아봐야할 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

해당 부분을 enter이벤트를 막기 보다는, title로 포커스를 강제 이동하는 방식으로 변경했습니다! 물론 title이 상호작용하진 않지만, 트리거 버튼에서 포커스를 해제시키는 게 좋을 것 같아서요

resolveConfirmation(false);
closeModal();
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 onDeny와 구분한 이유가 있나요?

}

if (key === 'Escape') {
resolveConfirmation(false);
closeModal();
}
}, []);
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.

isOpen 의존성에 추가해주었어요! 원래 ConfirmModal에 넣어주었다가 Provider로 옮기는 리팩터링 과정에서 실수했네요!


useEffect(() => {
if (isOpen) {
document.addEventListener('keydown', onKeyDown);
document.body.style.overflow = 'hidden';
}

return () => {
document.removeEventListener('keydown', onKeyDown);
document.body.style.overflow = 'auto';
};
}, [isOpen]);

return (
<ConfirmContext.Provider value={{ confirm }}>
{children}
{isOpen &&
createPortal(
<ConfirmModal
title={title}
content={content}
denial={denial ?? '닫기'}
confirmation={confirmation ?? '확인'}
onDeny={onDeny}
onConfirm={onConfirm}
/>,
document.body
)}
</ConfirmContext.Provider>
);
};

export default ConfirmProvider;
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 문든 궁금한 부분인데, Modal이라는 네이밍은 의도적으로 쓰지 않으신건가용?
파일명과는 다르게 ConfirmProvider, ConfirmContext 라고 되어있어서요~!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아뇨아뇨! 추가했습니다!

11 changes: 11 additions & 0 deletions frontend/src/shared/components/ConfirmModal/hooks/useConfirm.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { useContext } from 'react';
import { ConfirmContext } from '../ConfirmModalProvider';

export const useConfirm = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 이부분도 Modal이라는 네이밍은 일부러 제거하신건가요~?

개인적으로는 useConfirm이라고만 하면 Context를 사용하는 훅인지 모를수도 있을 것 같아요!
Modal이라는 네이밍이 붙으면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이름에 Context 추가했습니다!! 불필요하게 너무 길어지는 것 같아서 Modal은 넣지 않고 대신 반환 함수의 이름을 조금 더 자세하게 변경했어요!

const contextValue = useContext(ConfirmContext);
if (!contextValue) {
throw new Error('ConfirmContext Provider 내부에서 사용 가능합니다.');
}

return contextValue;
};