[WRFE-48] 래플/이벤트 수정 페이지 #50
Conversation
eric-hjh
force-pushed
the
feature/WRFE-48
branch
from
c37aa18 to
7b6f33f
Compare
eric-hjh
requested review from
ww8007,
sangminlee98,
gyeong3un2,
Ajeong-Im and
Joy0w0
| const Edit = ({ | ||
| params: {id}, | ||
| searchParams: {type}, | ||
| }: { | ||
| params: {id: string}; | ||
| searchParams: {type: 'raffle' | 'event'}; | ||
| }) => { |
There was a problem hiding this comment.
혹시 interface를 따로 선언하지 않는 이유가 따로 있으신가요?
저는 보통
interface Params {
id : string;
}
interface SearchParams {
type : 'raffle' | 'event'
}
interface EditProps {
params : Params;
searchParams : SearchParams;
}요런식으로 썼는데 interface 파일만 읽더라도 이 컴포넌트에서 대충 어느게 필요하겠구나에 대한
생각을 분리할 수 있어서 좋은 것 같았어요
이 부분에 대한 장현님의 생각은 어떠신가요?
There was a problem hiding this comment.
interface를 따로 선언하지 않는 이유는
- 나누지 않는게 더 직관적이다 생각하고,
- 개인적으로 코드를 읽을 때 아래 ->위 (왼 -> 오)로 읽는게 좋아서,
- 코드양이 굳이 늘아난다는 느낌을 가지고 있었습니다.
인터페이스로 분리할 때는 가독성이 떨어진다 느껴질 때 (ex. props로 받아오는게 많다거나, 복잡한 관계라던가..) 분리를 해서 사용해왔었는데,
'생각을 분리해 볼 수 있다' 와 같은 생각은 못해봤던 접근이어서 한번 생각해보겠습니다!
(이렇게 적었는데 밑에 훅 보니까 분리해서 사용했네요 ; 3개가 많다 생각하진 않는데)
There was a problem hiding this comment.
825710c
수정 커밋입니다!
다른 코드를 보았을 때, interface로 분리된 코드가 많아서 통일시키는편이 좋을거 같아서 분리시켜두었습니다!
apps/front/wraffle-webview/src/widgets/product-list/edit/ui/EditList.tsx
Outdated
Show resolved
Hide resolved
| export const useDateValidation = ({ | ||
| startDate, | ||
| endDate, | ||
| announceAt, | ||
| }: UseDateValidationProps) => { | ||
| const {setValue} = useFormContext(); | ||
| const {toast} = useToast(); | ||
|
|
||
| useEffect(() => { | ||
| if (endDate < startDate) { | ||
| setValue('endDate', startDate); | ||
| toast({ | ||
| title: '응모 마감 일정은', | ||
| description: '응모 시작 일정 이후로 설정해주세요.', | ||
| duration: 10000, | ||
| variant: 'warning', | ||
| }); | ||
| } | ||
|
|
||
| if (announceAt < endDate) { | ||
| setValue('announceAt', endDate); | ||
| toast({ | ||
| title: '당첨자 발표 일정은', | ||
| description: '응모 마감 일정 이후로 설정해주세요.', | ||
| duration: 10000, | ||
| variant: 'warning', | ||
| }); | ||
| } | ||
| }, [startDate, endDate, announceAt]); | ||
| }; |
There was a problem hiding this comment.
외부에서 사용할 때 이 컴포넌트가 어떤 일을 하는지 파일을 열어보기 전 까지는 동작을 예측하기 어려운 코드 같아요
- 날짜 판단
- 토스트
- form에 setting
useDateValidationWithToast 같은 네이밍을 가져가는 것도 좋을거 같은데 과연 form을 여기서 setting 하는게 훅의 책임이 맞는가에 대한 고민이 드네요
어떠신가요?
There was a problem hiding this comment.
코드 중복을 줄일 수 있는 점과 훅으로 나누긴 했지만 form을 사용하는 제한된 곳에서 사용할것이라는 생각으로 코드를 작성하여 간과한 부분인거 같아 수정하겠습니다
There was a problem hiding this comment.
a9c6708
수정 커밋 입니다!
There was a problem hiding this comment.
저번에 의논했었던 방향에서 처음 이야기 나왔던 onChange에 조건을 거는 방향으로 생각해보다가, 처음 생각했던 흐름은
- 오버된 날짜를 선택시 날짜를 강제로 변경 시킨 후, toast를 보여주는 방향이었는데,
생각해보니까, 날짜를 강제로 변경하는 흐름 보단, 선택할 때 유효성에 걸리면 토스트를 보여주고, 변경을 못하는 방향이 사용자에게 더 좋은 경험이 될거 같아서 이 방향으로 수정해 보았는데, 어떠신거 같나요??
4e1f418
There was a problem hiding this comment.
@eric-hjh
네 저는 이 방향도 좋을 것 같습니다!
| const Edit = ({ | ||
| params: {id}, | ||
| searchParams: {type}, | ||
| }: { | ||
| params: {id: string}; | ||
| searchParams: {type: 'raffle' | 'event'}; | ||
| }) => { |
There was a problem hiding this comment.
interface를 따로 선언하지 않는 이유는
- 나누지 않는게 더 직관적이다 생각하고,
- 개인적으로 코드를 읽을 때 아래 ->위 (왼 -> 오)로 읽는게 좋아서,
- 코드양이 굳이 늘아난다는 느낌을 가지고 있었습니다.
인터페이스로 분리할 때는 가독성이 떨어진다 느껴질 때 (ex. props로 받아오는게 많다거나, 복잡한 관계라던가..) 분리를 해서 사용해왔었는데,
'생각을 분리해 볼 수 있다' 와 같은 생각은 못해봤던 접근이어서 한번 생각해보겠습니다!
(이렇게 적었는데 밑에 훅 보니까 분리해서 사용했네요 ; 3개가 많다 생각하진 않는데)
| export const useDateValidation = ({ | ||
| startDate, | ||
| endDate, | ||
| announceAt, | ||
| }: UseDateValidationProps) => { | ||
| const {setValue} = useFormContext(); | ||
| const {toast} = useToast(); | ||
|
|
||
| useEffect(() => { | ||
| if (endDate < startDate) { | ||
| setValue('endDate', startDate); | ||
| toast({ | ||
| title: '응모 마감 일정은', | ||
| description: '응모 시작 일정 이후로 설정해주세요.', | ||
| duration: 10000, | ||
| variant: 'warning', | ||
| }); | ||
| } | ||
|
|
||
| if (announceAt < endDate) { | ||
| setValue('announceAt', endDate); | ||
| toast({ | ||
| title: '당첨자 발표 일정은', | ||
| description: '응모 마감 일정 이후로 설정해주세요.', | ||
| duration: 10000, | ||
| variant: 'warning', | ||
| }); | ||
| } | ||
| }, [startDate, endDate, announceAt]); | ||
| }; |
There was a problem hiding this comment.
코드 중복을 줄일 수 있는 점과 훅으로 나누긴 했지만 form을 사용하는 제한된 곳에서 사용할것이라는 생각으로 코드를 작성하여 간과한 부분인거 같아 수정하겠습니다
| @@ -0,0 +1,10 @@ | |||
| export const validateDate = ( | |||
There was a problem hiding this comment.
validateDate 보다는 이 날짜가 이전 날짜 보다 큰지를 보여주도록 함수 명에서 드러나면 좋을 것 같아요
isDateBefore 같은 느낌이 되면 어떨까요?
| onSelect={date => { | ||
| if (validateDate(date, announceAt)) { | ||
| field.onChange(date); | ||
| } else { | ||
| toast(ENDDATE_IS_OVER_ANNOUNCEAT); | ||
| } | ||
| }} |
There was a problem hiding this comment.
ui에서 이 로직을 담당하는 것 보다는 onSelect를 외부에서 받아서 관리하는 방식이 좋아보일 것 같아요!
추후에 이 로직을 수정하기 위해서는 ui에 해당하는 이 onSelect까지 와야 한다는 사실을 알기 어려울 것 같아요
| onSelect={date => { | ||
| if (validateDate(date, endDate)) { | ||
| field.onChange(date); | ||
| } else { | ||
| toast(STARTDATE_IS_OVER_ENDDATE); | ||
| } | ||
| }} |
📖 개요
💻 작업사항
💡 작성한 이슈 외에 작업한 사항
✔️ check list