-
Notifications
You must be signed in to change notification settings - Fork 5
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] 생성한 행사 모아보기 페이지 #856
base: fe-dev
Are you sure you want to change the base?
[FE] 생성한 행사 모아보기 페이지 #856
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
바쁜 와중에도 뚝딱 기능을 만들어내는 모습 멋있습니다~~ 고생했어요.
토다리가 논의해주신 재사용성 동의합니다. 토다리의 코드를 리뷰하면서 확실히 css 복사 붙여넣기한 내용이 많은 것 같더라구요... 제가 이해한 토다리가 원하는 재사용성을 높이는 방식은 디자인을 가진 컴포넌트에서 도메인 결합도를 낮추는 것이라고 이해했는데 맞을까요? 어떻게 해야할 지 논의를 해봐야겠지만 개발을 지속적으로 이어나간다고 하면 작업을 하는 것이 좋다고 생각해요!
Page 폴더도 확실히 정리가 되지 않은 느낌이 들어요. 이 정리는 위 정리보다 금방 할 것 같으니 회의 끝나고 아니면 회의 도중에 정리를 해서 그 상태에서 다시 개발을 시작해도 좋을 것 같습니다!
<Flex | ||
justifyContent="spaceBetween" | ||
alignItems="center" | ||
height="2.5rem" | ||
padding="0.5rem 1rem" | ||
paddingInline="0.5rem" | ||
> | ||
<Flex gap="0.5rem" alignItems="center" onClick={onClick}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확실히 그래요.. 이런 스타일을 복사 붙여 넣기 하면서 유지 보수가 어려워진다는 점 공감합니다.
이런 자주 사용하는 레이아웃을 따로 정의해서 가져온다면 개선할 수 있지 않을까 생각합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 아래 영역이 더 나을 것이라고 판단되네요.
위에 onClick을 넣도록 변경하겠습니다~
|
||
const useRequestGetCreatedEvents = () => { | ||
const {data, ...rest} = useQuery({ | ||
queryKey: [QUERY_KEYS.createdEvents], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
유저 로그아웃이 없으니깐 토다리가 작성해준 대로 이 부분은 queryKey 배열의 두 번째 인자 신경 쓰지 않아도 될 것 같아요!
queryFn: () => requestGetCreatedEvents(), | ||
select: data => ({ | ||
...data, | ||
events: data.events.sort((a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
select 좋은 것 같아요! 이 시간 비교는 밀리초 단위로 비교되는 건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아뇨 10^-6 까지 비교합니다 ㅋㅋ
@@ -11,7 +11,7 @@ const useRequestGetEvent = ({...props}: WithErrorHandlingStrategy | null = {}) = | |||
const eventId = getEventIdByUrl(); | |||
|
|||
const {data, ...rest} = useSuspenseQuery({ | |||
queryKey: [QUERY_KEYS.event], | |||
queryKey: [QUERY_KEYS.event, eventId], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
원래는 이벤트 생성을 했을 때 removeQueries를 사용해서 캐시를 전부 날려주었기 때문에 문제가 없었는데, 이제는 생성 외에 다른 이벤트 접근이 가능해서 일어난 문제였네요.. 이것 외에 또 다른 부작용은 없었나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 손수 테스트 한 케이스에선 별도로 없었습니다~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 좋아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생많으셨씁니다!! 이쁘게 구현이 되었네요!~~
토달이 pr에 써있는 대로 파일을 뷰어에서 찾기 어렵다는 것 이해합니당. 저는 특히 훅찾을 때가 어렵더라구요. 정리하면 좋겠네요~ 생각이 드는건 특정 페이지에서만 쓰이는 훅이나 컴포넌트를 함께 뭉쳐두는게 어떨까해요. 거기에서만 쓰이는건데 나와있으니까 다른 파일 찾는데 더 복잡해진 느낌이라..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구현해주신 부분 잘 봤어요! 꼼꼼하게 해주신 것 같아서 남길 코멘트는 딱히 없네요 ㅎㅎ 대신 토다리가 논의 요청한 부분에 대한 저의 생각을 말해드리자면..
많은 디자인 요소가 결정된 상황에서, 손쉬운 기능 추가 및 유지보수를 위해 component 재사용성을 높이고 싶다.
토다리가 말씀해주신 것 처럼 앞으로의 디벨롭을 생각하면 component 재사용성을 높이는 부분은 적극 찬성입니다. 그리고 저번에 우리가 디자인시스템을 폐기하면서 컴포넌트 폴더 구조가 약간? 이상한 것도 없지 않아 있구요. 다른 위치지만 컴포넌트 폴더가 두개 존재하니.. 이 부분은 추후 생성되는 기능이 없을 때 진행하면 정말 좋을 것 같아요!
쉬운 page 파일을 찾아내기 위해 route path, /Page 디렉토리, page 파일 이름에 대한 컨벤션을 정하고 싶다.
이 부분은 정말 정말 필요하다고 생각합니다.. 아무래도 route와 Page 폴더의 파일 네이밍이 다르다보니 찾는게 어렵더라구요...ㅜ 이 부분은 빠른 시일내에.. 진행하면 좋을 것 같습니댱!
height="100%" | ||
cssProp={{borderRadius: '1rem'}} | ||
> | ||
<Input inputType="search" value={eventName} onChange={onSearch} placeholder={placeholder} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
원하는 행사 서칭까지 가능하도록 해주시다니~ 👍
@@ -0,0 +1,12 @@ | |||
import {CreatedEvent} from './../../../../types/serviceType'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types/serviceType
로 import 할 수 없었나요?.?
@@ -11,7 +11,7 @@ const useRequestGetSteps = ({...props}: WithErrorHandlingStrategy | null = {}) = | |||
const eventId = getEventIdByUrl(); | |||
|
|||
const queryResult = useQuery({ | |||
queryKey: [QUERY_KEYS.steps], | |||
queryKey: [QUERY_KEYS.steps, eventId], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오우.. 다른 부분도 꼼꼼히 체크해서 eventId 넣어주셨군요! 쵝오
issue
구현 목적
구현 사항
route는 기존 @jinhokim98 이 구현한
mypage
를 이용하여,mypage/events
로 정했습니다.기존의
EventHomePage
과 비슷한 UI, 기능들로 인하여 이를 참고하여 구현하였습니다.피그마의 UX writing을 일부 수정하였습니다
기존 API는 오래된 것을 먼저 보여주지만,
useRequestGetCreatedEvents
hook 내부에서 data를 최신순으로 sort 하여 반환합니다.queryKey에 eventId 추가
기존 방식대로면, eventId가 변경되면서 다른 페이지에 접근해도, 기존의 요청 캐시로 인해 정보가 갱신되지 않았습니다. 이에 get 요청들의 query key에 eventId를 추가해 줌으로써, 페이지가 변경될 때 마다 새로 요청을 보내도록 변경했습니다.
개선 전
2024-12-16.7.14.31.mov
2024-12-16.7.18.01.mov
논의하고 싶은 부분(선택)
1. Component 재사용성에 대한 논의
기존의 ExpenseList와 비슷한 UI라고 생각하여 이를 재사용하려 하였으나, 이 조차도
Event
엔티티에 깊게 결합되어있어 재사용할 수 없었습니다.따라서, 비슷한 UI와 style 부분을 별도로 복사하여 작업하게 되었습니다.
계속 프로젝트가 진행될 때 마다, 이미 존재하는 디자인 요소를 그대로 사용함으로 인해서 확실히 생산성이 충분히 많이 향상되어 있다고 생각합니다.
하지만 반대로, 이와 같이 복사해서 사용하게 된다면 유지보수에 큰 어려움이 존재할 수 밖에 없다고 생각합니다.
앞으로 프로젝트를 얼마나 지속할지도, 디자인의 변경이 생겨 유지보수할 일도 예측이 불가능 하지만, 확실히 component들의 재사용성을 높여둔다면 지금까지 어느정도 필요한 component들이 모두 나온 상황에서 정말 최상의 재사용성을 유지할 수 있다고 생각합니다.
2.Page 폴더 디렉토리 및 route에 관한 논의
확실히 우리 서비스의 페이지가 많아지다 보니, 원하는 페이지 파일을 손쉽게 찾기 힘들어지고 있다고 느껴집니다.
Pages
디렉토리에 정말 많은 Page tsx 파일들이 있는데, 이를 route와 통일하여 논리적으로 쉽게 접근할 수 있는 방향을 함께 논의해보면 어떨까 싶습니다. 이런 작은 convention 하나하나가 모여 높은 생산성을 가진 협업의 기초가 되지 않을까 싶습니다...!만약 논의가 되고 난 후, 새로운 페이지가 생기는 task를 가져갈 때, 해당 페이지의 route 및 directory를 함께 정하고 작업에 들어가도 좋을 것 같습니다!
결론
route path
,/Page 디렉토리
,page 파일 이름
에 대한 컨벤션을 정하고 싶다.🫡 참고사항