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

[FE] 회원 행사 생성, 비회원 행사 생성 구현 #835

Merged
merged 26 commits into from
Nov 20, 2024

Conversation

pakxe
Copy link
Contributor

@pakxe pakxe commented Nov 19, 2024

issue

구현 목적

소셜 로그인이 추가됨에 따라 행사 생성 과정에서 비밀 번호, 관리자 이름을 입력받지 않을 수 있게 되었습니다.
따라서 회원이 행사를 생성하는 경우는 행사 이름만 입력하면 바로 행사를 생성할 수 있도록 구현했습니다.

다만 비회원일 경우 행사에서 사용할 관리자 이름(닉네임)을 입력받는 스텝 페이지가 추가되었습니다.

  • 비회원 행사 생성: 행사 이름 입력 -> 관리자 이름 입력 -> 비밀번호 입력 -> 완료
  • 회원 행사 생성: 행사 이름 입력 -> 완료
image

용어 설명

  • 스텝 페이지: 퍼널 구조에서 사용되는 스텝. 페이지 모습을 갖고 있기 때문에 스텝 페이지라고 부르겠습니다.

구현 사항

회원 행사 생성 페이지와 비회원 행사 생성 페이지의 path를 다르게하여 진입시킵니다.

  • 회원 행사 생성 페이지: /event/create/guest
  • 비회원 행사 생성 페이지: /event/create/member
image

점선이 그려져있는 완료 스텝 페이지는 두 퍼널 모두 동일한 스텝 페이지를 사용합니다.

다만 행사 이름을 입력받는 스텝 페이지는 두 퍼널이 다른 스텝 페이지를 사용합니다.

사실 두 가지 선택지가 있었는데요.

  1. 분리하지 않고 하나의 행사 이름 스텝 페이지를 사용하되 다음 버튼을 눌렀을 때 실행할 콜백을 외부에서 주입받기
  2. 분리하고 행사 이름 스텝 페이지 내부에서 비회원 행사 생성 또는 회원 행사 생성 api를 호출하기

1번을 선택하지 않은 이유는 퍼널 구조에서 각 스텝이 수행해야하는 스텝 로직을 퍼널 컴포넌트로 올려서 내려보내는게 과연 옳은가? 라는 질문에 대해서 옳지 않다고 생각했기 때문입니다. 스텝이 10개 이상 될 경우 퍼널 컴포넌트 자체에 스텝에서 사용하는 로직이 10개나 되어야 한다는 뜻입니다. 그럼 페이지 레이아웃이 바뀌면 유지보수가 어렵지 않냐 라고 생각할 수 있는데요. 스텝 페이지의 레이아웃이 크게 변화될 가능성은 높지 않다고 생각했고, 만약 그런 순간이 온다면 컴포넌트를 잘 만들어서 해결할 수 있지 않을까 싶습니다.

그리고 1번으로 구현했을 경우 디버깅이 어려워집니다. 다음 버튼을 눌렀을 때 실행하는 로직이 외부에 있으니까요.

결국 페이지 복붙으로 새로운 페이지를 만드는 것이 재사용성을 낮게하지않나? 로직을 주입받으면 되지않냐? -> 재사용성은 컴포넌트를 잘 설계한다면 복붙 컴포넌트라도 유지보수 난이도를 낮게 할 수 있음. 그리고 실행되는 로직을 상위에 두는 것이 책임 분산

그래서 2번으로 구현했습니다.

다른 선택지도 추천해주시면 고민해보겠습니다!!

참고사항

이미 만들어진 로직을 재사용하도록 위해 리펙터링된 부분이 있습니다.

pakxe added 18 commits November 19, 2024 18:39
@pakxe pakxe added 🖥️ FE Frontend ⚙️ feat feature labels Nov 19, 2024
@pakxe pakxe added this to the v2.2.0 milestone Nov 19, 2024
@pakxe pakxe self-assigned this Nov 19, 2024
@pakxe pakxe linked an issue Nov 19, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Nov 19, 2024

Test Results

 25 files   25 suites   7s ⏱️
147 tests 147 ✅ 0 💤 0 ❌
151 runs  151 ✅ 0 💤 0 ❌

Results for commit f132106.

♻️ This comment has been updated with latest results.

Copy link

Copy link
Contributor

@jinhokim98 jinhokim98 left a comment

Choose a reason for hiding this comment

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

고생했어요~~ 웨리! 저는 두 개로 분리해서 만드는 것이 더 좋다고 생각했어요. 이것저것 확장해서 컴포넌트에 분기처리를 하다보면 예상치 못한 버그를 마주할 수 있다고 생각해요. (물론 지금은 회원만 추가되는거지만) 깔끔하게 플로우를 나누어서 보여주는 것이 더 좋다고 생각합니다.

cy.get('button').contains('행동 개시!').click();
});
});
// it('랜딩페이지에서 "정산 시작하기" 버튼을 눌러 행사 이름 입력 페이지로 이동해야 한다.', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

cypress를 주석처리하셨군요ㅋㅋ 테스트 보다는 구현이 먼저라 생각하기 때문에 정상적으로 돌아간다는 전제 하에 임시로 주석처리 괜찮은 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

@jinhokim98 이 부분은 쿠키가 작성한 부분이 반영되면 해결되는것인가요??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 이슈 하나만 있다면 구현했을텐데, 로그인에서 해야할 이슈가 좀 많아서 임시로 해두었습니다 ㅠ.ㅠ

Copy link
Contributor Author

@pakxe pakxe Nov 19, 2024

Choose a reason for hiding this comment

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

쿠키 기능이 완성되면 회원과 비회원 각각의 e2e 테스트를 모두 작성해야 할 것 같네요 🙋‍♀️ 다만, 로그인과 행사 생성 플로우보다 프로젝트의 핵심 기능에 우선적으로 테스트를 작성하는 것이 더 효율적일 수도 있다고 생각합니다. (애초에 지금 e2e가 행사 생성 과정 말고는 없어서.. ㅜㅜ) 취준으로 인해 프로젝트에 사용할 시간이 제한적인 만큼, 무엇을 위해 테스트를 작성하는지를 명확히 정리한 후, 어떤 테스트가 더 중요한지 고민해보는 것이 좋을 것 같아요 👍

@@ -47,7 +47,7 @@ Cypress.Commands.add('interceptAPI', ({type, delay = 0, statusCode = 200}: Inter
});

Cypress.Commands.add('createEventName', (eventName: string) => {
cy.visit(ROUTER_URLS.createEvent);
cy.visit(ROUTER_URLS.createMemberEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분 충돌 날 거예요. 제 부분에서 로그인 진입점도 있어서 이 코드 수정했거든요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

예상했습니다 ㅎㅎ 즐거운 충돌쇼

eventName: string;
password: string;
}
export const requestPostGuestEvent = async (postEventArgs: CreateEventArgs) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

args라는 변수명 좋네요! props가 익숙해서 props로 지은 것도 많은데 함수의 파라미터이니깐 args가 더 좋은 이름인 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

api type 이름에 대해서 예ㅔㅔ전에 용어정리 하면서 함께 통일했던 기억이 있어요
https://pakxe.notion.site/8455260f33074f7fbf0fbdf55b187e2f?pvs=4
사실 최근들어서 다들 생각하지 않고 그냥 코드를 작성하고 있는것 같은데,
그때도 유지보수성을 위해서 모두가 같은 context에서 이해할 수 있도록 수정했는데, 최대한 맞추는게 이후에도 보기 좋고 이해하기 좋을 것이라고 생각돼요.

이전에 작업한 convention은 팀과의 협업 중 불편하다고 생각되어 만들어 진 것이니까요~!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저는 컴포넌트의 인자로 들어가는 값이면 props, 함수의 인자로 들어가는 값이면 args라고 생각하며 구현했었어요!

@@ -1,15 +1,20 @@
const EVENT = '/event';
const EVENT_WITH_EVENT_ID = `${EVENT}/:eventId`;
Copy link
Contributor

Choose a reason for hiding this comment

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

event with event id 처음 들었을 때 어색하긴 했는데 어쩔 수 없는 것 같아요 id라고 줄이면 무슨 id인지 혼동할 수 있으니

Copy link
Contributor Author

Choose a reason for hiding this comment

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

흑흑... 더 생각해볼게요

import validateMemberName from '@utils/validate/validateMemberName';
import {NickName} from 'types/createEvent';

type UseSetNicknameStepProps = ReturnType<typeof useSetNicknameStep>;
Copy link
Contributor

Choose a reason for hiding this comment

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

ReturnType 좋아요! 훅의 리턴이 너무 많을 때 이렇게 쓰면 유용하더라구요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

마자요. 자동 타입 추론을 최대한 활용하는게 이득이죵

// 실행 순서를 await으로 보장하기 위해 mutateAsync 사용
return {
postEvent: mutateAsync,
isPostEventPending: rest.isPending,
Copy link
Contributor

Choose a reason for hiding this comment

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

pending을 내보내주는 이유는 버튼의 로딩 애니메이션을 보여주기 위한 것일까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어, 있던 코드를 그대로 복붙해온거라 몰랐는데 적용시키면 되겠네요. 적용 완료했습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

굳뜨~~

Comment on lines +51 to +56
const trackCompleteCreateMemberEvent = (eventUniqueData: EventUniqueData) => {
track('회원 이벤트 생성 완료', {
...eventUniqueData,
});
};

Copy link
Contributor

Choose a reason for hiding this comment

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

amplitude까지 꼼꼼하게 좋습니다~~ 제 기능에서도 회원 행사 생성인지 비회원 행사 생성인지 구분해서 받고 있거든요

@@ -35,10 +34,10 @@ const SetEventPasswordStep = ({eventName, moveToNextStep, setEventToken}: SetEve
>
<Top>
<Top.Line
text={`관리에 필요한 ${RULE.maxEventPasswordLength}자리 숫자`}
text={`행사에서 사용할 ${RULE.maxEventPasswordLength}자리 숫자`}
Copy link
Contributor

Choose a reason for hiding this comment

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

"행사에서 사용할"이라 바꾼 이유가 궁금해요!

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 궁금하네용 🧐

Copy link
Contributor

Choose a reason for hiding this comment

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

아마 SetNickName에서 사용한 것 처럼 행사에서 사용할 ~~ 로 맞추고 싶었던 것 같은데,
약간 어색한 감은 있어요. 관리자의 비밀번호도, 관리자의 이름도 행사에서 사용되는 개념은 아닌 것 같아서
행사를 정산하는 관리자의 이름을 입력해 주세요, 행사를 정산하는 관리자의 비밀번호를 입력해 주세요 와 같은 식으로 작성하는게 어떨까 싶어요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"관리에 필요한 네자리 숫자의 관리자 비밀번호는 뭐라고 할까요?"에서 관리가 겹치니까 뭔가뭔가 어색해서 바꾼건데, 바꾼게 이상하다면 다시 돌려놓을게용

Copy link
Contributor

Choose a reason for hiding this comment

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

듣고 보니 그럴 수도 있을 것 같아요!
내일 회의 때 논의해봅시다~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오호 좋아요 토다리 말대로 바꿔볼게요〰️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

아아아악 더 좋은게 있을 것 같은데 떠오르지 않네요 ㅜㅜㅜㅜ
그래도 적극적으로 반영해줘서 고마워요 웨디

errorText={errorMessage ?? ''}
value={nickname}
type="text"
placeholder="홍길동"
Copy link
Contributor

Choose a reason for hiding this comment

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

플레이스홀더 박세현으로 어떤가요?ㅋㅋㅋㅋㅋㅋㅋ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋㅋㅋ 장난쓰😅😅

const {postEvent} = useRequestPostMemberEvent();
const {trackCompleteCreateMemberEvent} = useAmplitude();

const submit = async (event: React.FormEvent<HTMLFormElement>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

사소한 코멘트이지만 메서드 명 onSubmit 어떤지 제안드려요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

submit으로 쓰이는 곳이 있어서 그렇게 해봤는데, 역시 on이 붙은게 좀 더 직관적인가요? 바꿔두었어요.

Copy link

Copy link

@pakxe pakxe changed the base branch from main to fe-dev November 19, 2024 12:27
Copy link

Copy link
Contributor

@Todari Todari left a comment

Choose a reason for hiding this comment

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

복잡한 부분이 많았을 텐데 구현도 잘 해주시고, 설명도 잘 작성해 주셔서 코드를 쉽고 빠르게 이해할 수 있었어요 고마워요 웨디~!

처음에는 guest와 member 두가지 경우에서 같은 Funnel을 사용하는게 맞지 않나라는 생각이 들긴 했는데, 웨디가 작성한 이유가 꽤나 근거있고 합당해서 이대로 진행해도 좋을 것 같아요!

다만, 두가지 경우의 url이 /event/create/guest, /event/create/member로 다른것은 반대하는 입장입니다.

우선 똑같은 역할을 가진 페이지인데 URL이 다르면 논리적으로 경로를 추적하는데 혼란스러울 뿐 아니라 통용적으로 사용되는 URL convention과도 맞지 않기 때문이에요. 웨디가 작성해 준 /event/create/guest, /event/create/member` 을 제가 흔히 접하는 url대로 해석해보고자 한다면, event가 있고, event에서 create할 수 있는 상위 페이지가 있고 그 하위에 guest와 member라는 어떠한 객체를 생성하는 도메인처럼 생각돼요.

두번째로는, GA, amplitude 에서 기본적으로 path 별로 session을 트래킹 하는 기본 로직이 있기 때문이에요, 지금 대로 path를 나눠놨다면 이벤트 생성에 대한 이벤트가 guest와 member 각각 두개로 나뉘게 될 것 같아요.

이런 이유들로 같은 url을 사용하고, 내부 로직을 통해서 렌더를 다르게 하는게 맞다는 생각이 듭니다!

실제로 영화예매 사이트 등 비회원으로 어떠한 것을 생성(영화예매의 경우엔 예매)하는 과정에서 로그인 했을 때와 같은 path에서 진행되는것을 확인할 수 있었습니다~

cy.get('button').contains('행동 개시!').click();
});
});
// it('랜딩페이지에서 "정산 시작하기" 버튼을 눌러 행사 이름 입력 페이지로 이동해야 한다.', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jinhokim98 이 부분은 쿠키가 작성한 부분이 반영되면 해결되는것인가요??

eventName: string;
password: string;
}
export const requestPostGuestEvent = async (postEventArgs: CreateEventArgs) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

api type 이름에 대해서 예ㅔㅔ전에 용어정리 하면서 함께 통일했던 기억이 있어요
https://pakxe.notion.site/8455260f33074f7fbf0fbdf55b187e2f?pvs=4
사실 최근들어서 다들 생각하지 않고 그냥 코드를 작성하고 있는것 같은데,
그때도 유지보수성을 위해서 모두가 같은 context에서 이해할 수 있도록 수정했는데, 최대한 맞추는게 이후에도 보기 좋고 이해하기 좋을 것이라고 생각돼요.

이전에 작업한 convention은 팀과의 협업 중 불편하다고 생각되어 만들어 진 것이니까요~!

const queryClient = useQueryClient();

const {mutate, mutateAsync, ...rest} = useMutation({
mutationFn: ({eventName, password}: RequestPostEvent) => requestPostEvent({eventName, password}),
mutationFn: (eventName: EventName) => requestPostMemberEvent(eventName),
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분도 위에서 말했던, API type에 대한 convention을 지키는게 좋을 것 같습니당

Copy link
Contributor Author

@pakxe pakxe Nov 19, 2024

Choose a reason for hiding this comment

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

serviceType파일에 api관련된 타입을 적으라는 의미가 맞을까요? 토달스?! 이렇게 수정해봤습니당.

export type EventName = string;
export type NickName = string;
export type Password = string;

export interface EventCreationData {
  eventName: EventName;
  nickname: NickName;
  password: Password;
}

// 원래 있던거.
export interface Event {
  eventName: EventName;
  bankName: string;
  accountNumber: string;
}

Event라는 타입은 원래 있던 타입인데, 이벤트에 왜 계좌이름과 계좌번호라는 타입이 붙어있는진 잘 모르겠으나, 의도한 게 있을 것 같아 남겨뒀어요. 그래서 이벤트 생성시에 반복적으로 사용되는 타입인 행사명, 닉네임, 비밀번호를 담아 EventCreationDara라고 만들어봤습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

저희가 그때 의논한 바로는, src/apis/request 에서는 requst + post + 요청하는 method
이런식으로 네임을 지었던 것으로 기억해요. 백엔드 측에서도 나름 RESTful하게 API들을 수정했고, 저희도 그 컨벤션에 비슷하게 맞추면
한 페이지에서 이해가 깔끔하게 되는게 그 취지였구요!

다른 src/apis/request 파일들을 확인해 보시면, requestDeleteMember, requestPostBill 과 같이 작성된 것을 확인할 수 있을거에요~!!

</Funnel.Step>

<Funnel.Step name="adminName">
<SetNickNamePage moveToNextStep={moveToNextStep} {...nickNameProps} />
Copy link
Contributor

Choose a reason for hiding this comment

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

다른 Funnel 하위의 component는 Step인데, SetNickNamePage인 이유가 있을가요?
설명에서 작성한

스텝 페이지: 퍼널 구조에서 사용되는 스텝. 페이지 모습을 갖고 있기 때문에 스텝 페이지라고 부르겠습니다.

부분이 SetNickNamePage 에 대한 설명인 것 같은데 이해가 되지 않아서요.
페이지 모습을 갖고 있다는게 무엇인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

미스테이크!! 입니다.. Step이 맞아요 ㅎㅎ 수정하겠습니다 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

아핫! 확인했습니다 :)


import {FixedButton, Input} from '@HDesign/index';

import RULE from '@constants/rule';

type SetEventPasswordPageProps = {
eventName: string;
type SetEventPasswordPageProps = Omit<CreateEventArgs, 'password'> & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Component의 이름은 SetEventPasswordStep 인데 type 이름은 SetEventPasswordPage 인 이유가 있을까요???

Copy link
Contributor Author

@pakxe pakxe Nov 19, 2024

Choose a reason for hiding this comment

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

이름이 길어지다보니 제대로 작성되었는지 파악이 잘 안되네요. 봐주셔서 감사해요 😆👍 이것도 Step이 맞습니다 ㅎ,ㅎ

Copy link
Contributor

Choose a reason for hiding this comment

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

좋아용~!~!

@@ -35,10 +34,10 @@ const SetEventPasswordStep = ({eventName, moveToNextStep, setEventToken}: SetEve
>
<Top>
<Top.Line
text={`관리에 필요한 ${RULE.maxEventPasswordLength}자리 숫자`}
text={`행사에서 사용할 ${RULE.maxEventPasswordLength}자리 숫자`}
Copy link
Contributor

Choose a reason for hiding this comment

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

저도 궁금하네용 🧐

@@ -35,10 +34,10 @@ const SetEventPasswordStep = ({eventName, moveToNextStep, setEventToken}: SetEve
>
<Top>
<Top.Line
text={`관리에 필요한 ${RULE.maxEventPasswordLength}자리 숫자`}
text={`행사에서 사용할 ${RULE.maxEventPasswordLength}자리 숫자`}
Copy link
Contributor

Choose a reason for hiding this comment

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

아마 SetNickName에서 사용한 것 처럼 행사에서 사용할 ~~ 로 맞추고 싶었던 것 같은데,
약간 어색한 감은 있어요. 관리자의 비밀번호도, 관리자의 이름도 행사에서 사용되는 개념은 아닌 것 같아서
행사를 정산하는 관리자의 이름을 입력해 주세요, 행사를 정산하는 관리자의 비밀번호를 입력해 주세요 와 같은 식으로 작성하는게 어떨까 싶어요!

Copy link
Contributor

@soi-ha soi-ha left a comment

Choose a reason for hiding this comment

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

잘 만들어줘서 고마워요 웨디~@!!

Comment on lines +35 to +58
return (
<div
css={css`
display: flex;
flex-direction: column;
gap: 1rem;
padding: 1rem;
`}
>
<Top>
<Top.Line text="정산을 시작하려는" />
<Top.Line text="행사의 이름은 무엇인가요?" emphasize={['행사의 이름']} />
</Top>
<form onSubmit={onSubmit}>
<Input
labelText="행사 이름"
errorText={errorMessage ?? ''}
value={eventName}
type="text"
placeholder="행동대장 야유회"
onChange={handleEventNameChange}
isError={!!errorMessage}
autoFocus
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

다음 버튼을 제외한 부분은 PR 코멘트에서 말씀해주신 것처럼 모두 동일하니, 해당 부분만 따로 컴포넌트로 분리하는 건 어떨지 의견 드려요! 그러면 만약 한쪽에서 디자인이 변경되더라도 하나의 컴포넌트에서만 수정하면 되니까 유지보수하기 더 쉬울 것 같아서용

Copy link
Contributor Author

@pakxe pakxe Nov 19, 2024

Choose a reason for hiding this comment

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

<div
      css={css`
        display: flex;
        flex-direction: column;
        gap: 1rem;
        padding: 1rem;
      `}
    >

위 부분은 프로젝트 내의 모든 퍼널의 모든 스텝에서 반복되기 때문에 컴포넌트로 만드는 것이 좋다는 것은 동의합니다!! 기술 부채를 갚을 때가 왔네요.

저는 저 부분을 제외하고 컴포넌트로 분리하기에는 일단 재사용되는 횟수가 2회 뿐이고(회원 행사 생성, 비회원 행사 생성) 복잡한 페이지가 아니라서 유지보수 어려움이 상당한가? 라는 생각은 잘 안들었던 것 같아요. 그치만 만약 변경 요구사항이 생기면 최대 소요 시간 = (하나 고치는데 드는 시간) *2 이니까요. 아래처럼 컴포넌트로 만드는 것을 대략적으로 생각해보았는데요. 어떤가요?!

// 재사용을 위해 분리된 컴포넌트
const CreateEventNameStepBody = ({onSubmit, ...props}) => {
  return (
    <Top ...>
    <form onSubmit={onSubmit}>
      <Input
          labelText="행사 이름"
          type="text"
          placeholder="행동대장 야유회"
          autoFocus
          {...props}
      />
    </form>
  )
}

// 위 컴포넌트를 사용하는 모습
// 비회원 행사 생성인 경우 퍼널이라는 상위에서 스텝의 하위로 데이터를 내려줘야 하기 때문에 useEventNameStepReturns라는 값을 넘겨줘야함..
const 행사명입력스텝 = ({버튼클릭시 실행할 로직콜백, useEventNameStepReturns}) => {  
  return (
    <FunnelStepLayout>
      <CreateEventNameStepBody onSubmit={...} onChange={handleEventNameChange} isError={...}   다양한 props들 />
      <Button onClick={버튼클릭시 실행할 로직콜백}/>
    </FunnelStepLayout>
  )
}

소하가 의도한게 이게 맞을까요? 맞는진 모르겠지만 추측한대로 수도코드를 짜고보니 변수명이 바뀌면 번거롭겠다 생각이 들기도 합니다. 컴포넌트를 쪼개개되면 Funnel -> Step -> StepBody 로 useEventNameStepReturns라는 props가 흐르니까요. 컴포넌트로 나누던 나누지 않던 둘 다 장단점이 비등비등하다고 생각되어요. 혹시 컴포넌트 분리에 있어서 제가 찾지 못한 좋은 장점이 있을까요?

@pakxe
Copy link
Contributor Author

pakxe commented Nov 19, 2024

@Todari 회원 행사 생성, 비회원 행사 생성의 path를 동일하게 유지하는 영화 사이트를 알려주실 수 있나요? 방식을 따라하고 싶은데요. 동일하게 유지하는 방법이 다양해서 ..

일단 회원 행사 생성, 비회원 행사 생성의 path를 동일하게 할 수 있는 방법에 대해 몇 가지 생각해보았는데요. 회원이냐? 라는 정보를 어떤 형태로는 행사 생성 페이지에 넘겨주는 방법이 있습니다.

특히나 고려했던 부분은 행사 생성 중 새로고침을 했을 때 어떻게 동작하느냐 인데요. 예를들어 카카오 로그인을 마친 상태에서 회원 행사 생성을 하고 있는데, 네트워크가 갑자기 안되서 다시 연결하고 새로고침이 되었을 경우 회원 행사 생성 페이지에 그대로 머무를 수 있도록 하고싶었어요.

장점 단점
세션 스토리지 path유지됨, 새로고침 시에도 유지됨 로그인 페이지에서 사용되는 훅에 행사 생성시에만 사용하는 세션 코드가 입혀지기 때문에 좋지 않음.
router state path유지됨 새로고침 시 날아감
query parameter 새로고침 시에도 유지됨. url 똑같지 않게됨

어떤 방법을 사용해도 사실 최적의 해결책은 없었습니다. 코드가 더럽냐 vs 새로고침 시 날아가도록 두드냐 인데, 코드 더러움의 수준이 견딜 수 있는 수준 이상일 것 같아서 새로고침을 했을 때 다른 방식으로 동작하는건 어떤지 고민을 해보았습니다.

그래서 생각난 방법이 있는데요. 일단 세션 스토리지 방식을 사용하는데, 비회원으로 생성하기 버튼을 눌렀을 경우 세션에 비회원으로 행사생성중정보를 추가합니다.

그리고 행사 생성 퍼널은 세션을 가져와 비회원으로 행사생성중정보가 있을 경우 비회원 행사 생성 퍼널을 렌더링합니다. 정보가 저 값이 아닐 경우 회원 행사 생성 페이지로 보냅니다. 이렇게 구현했을 경우에 생각나는 모든 경우에서 그나마 원활하게 구현될 것 같아요.

  1. 비회원 행사 생성 버튼 클릭 -> 비회원 행사 생성 중 새로고침 -> 세션에 비회원으로 행사생성중 정보가 남아있기 때문에 비회원 행사 생성 퍼널 렌더링 가능 -> 정상적으로 행사 생성을 완료한다면 세션 클리어
  2. 카카오 로그인 정상 진행 -> 회원 행사 생성 중 새로고침 -> 세션에 뭐가 없는 경우는 회원 행사 생성 퍼널 렌더링하므로 문제없음
  3. 카카오 로그인 진행 x(쿠키없음) && url에 행사 생성 페이지로 바로 진입 -> 회원 행사 생성 페이지로 유도되지만 쿠키가 없기 때문에 권한 에러 발생 -> 권한 에러는 로그인 페이지로 이동됨 -> 카카오 로그인 || 비회원 진행 -> 카카오 로그인을 선택하여 행사 생성 -> 회원 가입 유도됨
  4. 카카오 로그인 진행 o(쿠키 있음) && url에 행사 생성 페이지로 바로 진입 -> 2번의 동작과 동일

다만 이 3개의 상황 모두 퍼널에서 사용된 데이터를 유지하는 로직이 필요하기 때문에 이 PR이 상당히 커질 것 같은데요. 지금 path가 다른게 직관적이지 않을 뿐 기능으로는 문제가 없기 때문에 다른 이슈로 만들어서 작업해도 될까요?

@pakxe pakxe requested a review from Todari November 19, 2024 18:46
@Todari
Copy link
Contributor

Todari commented Nov 20, 2024

@pakxe
역시 이게 웨디의 최대 장점이 아닐까 싶어요. 꼼꼼하게 다양한 케이스를 분석해 줘서 너무 감사합니다!

우선 제가 비회원으로 무언가를 생성하는게 뭐가 있을까 e커머스, 티켓예매, 영화를 다 찾아봤는데 요즘은 영화예매 말고는 회원밖에 안되더라구요
그 중 CGV의 경우 로그인을 하거나, 비회원으로 계속하기를 통해서 같은 path의 영화 예매 페이지에 들어가요. 이후 프로세스는 그대로 진행됩니다!
아마 추측하기로는, 로그인 관련 정보(비회원 포함)가 쿠키에 저장되는 것이 아닌가 싶어요~!

정리해 주신 대로, 쉽게 한다면 queryParameter를 추천드리고 싶긴 했으나, 우리 서비스에서 queryParameter를 사용하는 곳이 아무곳도 없다 보니까 그 부분도 염려되긴 하더라구요.

사소한 의견으로 넘길 수 있는 PR인데 꼼꼼하게 파악해 줘서 다시 한번 너무 감사하고, 웨디가 여유가 된다면 작업해줘도 좋을 것 같아요!
오늘 회의에서 굳이 path가 같아야하나? 라는 논의를 진행해 보고 다른 크루들이 반영할 필요가 없다고 느낀다면 진행하지 않아도 좋을 것 같습니다!!!
웨디가 깔끔하게 정리해 준 덕분에, 간접적으로 어떻게 구현해야 할 지 다들 어느정도 이해할 수 있게 된 것 같으니까요~!

Copy link

Copy link

@jinhokim98 jinhokim98 merged commit 40c8804 into fe-dev Nov 20, 2024
2 checks passed
@jinhokim98 jinhokim98 deleted the feature/#834 branch November 20, 2024 06:56
jinhokim98 pushed a commit that referenced this pull request Dec 11, 2024
* feat: 이벤트 생성에서 반복적으로 사용되는 타입 선언

* refactor: 반복되는 path를 상수로 만들고 이를 재사용함

* feat: 게스트 행사, 회원 행사 생성을 위한 api 연결 함수 구현

* feat: 유저 이름이 올바르지 않을 때 보여줄 에러 메세지 수정

* refactor: 이미 제작된 이름 유효성 검증 로직을 재사용하도록 수정

* feat: 행사 생성 시 관리자 이름을 입력받는 페이지 구현

* feat: 회원 이벤트 생성 과정 퍼널 구현

* refactor: 불필요한 조건문 제거

* feat: 비회원 행사 생성 페이지, 회원 행사 생성 페이지를 라우터에 연결

* refactor: 행사 생성시 사용되는 타입으로 재사용하도록 수정

* feat: 회원 이벤트 생성을 트래킹할 amplitude 함수 구현

* feat: 회원 이벤트 생성 시 행사명만 받으면 바로 행사를 생성하는 플로우로 스텝 페이지 구현

* chore: 행사 생성 과정이 회원 여부로 분기됨에 따라 위치와 파일명 수정

* feat: 행사 생성 시 행사 관리자의 이름을 입력받는 스텝 페이지에서 사용하는 이름 검증 훅 구현

* chore: 행사 생성에 대한 path 경로를 수정

* chore: 사용하지 않게된 파일 제거

* test: 분리 개발로 인해 테스트 수행 불가하므로 테스트코드 주석처리

* rename: nickName -> nickname으로 수정

* feat: 사용하지 않는 return 함수 제거

* feat: 회원 이벤트 생성 완료 버튼을 누를 때 로딩이 되도록 함

* rename: submit->onSubmit으로 form태그에서 사용되는 핸들러이름과 일치시킴

* feat: 행사 생성 시 입력받는 관리자 이름, 비밀번호 스텝 페이지의 문구 변경

* rename: Page -> Step으로 이름 변경

* feat: api에서 사용되는 타입을 serviceType으로 이동

* rename: NickName -> Nickname으로 수정
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[FE] 회원 행사 생성, 비회원 행사 생성 구현
4 participants