-
Notifications
You must be signed in to change notification settings - Fork 4
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
배너 공지사항 조회 / 공지사항 목록 조회 / 공지사항 상세 조회 / 공지사항 수정 / 공지사항 삭제 / 공지사항 생성 api 구현 및 query hook 구현 #739
Conversation
⚡️ Lighthouse report!
|
상세 게시글에서 이미지를 눌렀을 때 이미지가 확대되어져 보이는 모달이 보이도록 구현
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.
타입 관련 코멘트 남겼습니다!!
공지사항 관련 API가 많은데 빠르고 꼼꼼한 구현 대단합니다👍👍
getBannerNotice, | ||
{ | ||
suspense: true, | ||
cacheTime: 30 * 60 * 1000, |
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.
👍👍
frontend/src/types/notice.ts
Outdated
export interface NoticeList { | ||
totalPageNumber: number; | ||
currentPageNumber: number; | ||
noticeList: Notice[]; |
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.
export interface NoticeListResponse {
totalPageNumber: number;
currentPageNumber: number;
notices: NoticeResponse[];
}
api/notice 에 이런 타입이 있던데
export type NoticeList = Omit<Notice, 'notices'> & { noticeList: Notice[] };
처럼 유틸리티 타입으로 기존 타입인 Notice를 활용해보는 것도 좋겠네요!
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.
제로 타입의 마술사이신가요? 🌊
그렇지만 바꾸지 않는 것이 더 유지 보수에 좋다고 생각이 들었습니다 그 이유는 아래와 같습니당
totalPageNumber와 currentPageNumber와 같은 명세가 백엔드에서 고쳐도 프론트에서는 고정이라고 생각을 하는데요. 유틸리티 타입으로 했을 때 오히려 수정하는 코드가 더 많아진다고 생각이 드는데 어떻게 생각하세요?
아래와 같이 api의 NoticeListResponse만 변경되어도 되는 코드가 제안해주신 코드로 한다면 프론트 코드에게 영향을 준다고 생각이 들어요
currentPageNumber가 pageNumber로 바뀌었을 경우 예시
export type NoticeList = Omit<Notice, 'notices' | 'pageNumber'> & { noticeList: Notice[] ; currentPageNumbre : number; };
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.
음 우스의 의견도 어느정도 공감이 되는군요!!
저는 Omit으로 제외하는 타입이 3개까지는 괜찮다고 스스로 합의하고.... 현재 신고 목록 조회 API 관련 타입을 이미 선언한 상태인데요,
key 이름이 바뀌는게 3개 이내라면 앞으로도 Omit으로 처리할거 같아요!
그렇지만 만약 백엔드 측에서 대규모 리팩터링을 해버려서 키값들이 다 바뀐다면..?? 우스의 방식대로 하는 것이 합리적이라고 생각합니다😀
이건 수아의 의견도 궁금하군요 @chsua
return data; | ||
}, | ||
onError: () => { | ||
console.error('공지 사항을 리스트를 불러오는데 실패했습니다'); |
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.
알려주셔서 감사합니다 👍👍👍
frontend/src/api/notice.ts
Outdated
notices: NoticeResponse[]; | ||
} | ||
|
||
export type NoticeRequest = Omit<NoticeResponse, 'createdAt' | 'id'>; |
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.
id 제외하시는 이유가 궁금해요!!
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.
공지사항 생성 시 id는 받지 않기 때문에 제외시켰습니당
{
title: string, (100자)
content: string, (3000자)
deadline: datetime (DHM 형태이다, yyyy-mm-dd HH:MM) : string
bannerTitle : string, (100자)
bannerSubtitle: string (100자)
}
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.
아하 공지사항 생성과 달리 공지사항 수정할 때는 noticeId 를 url의 params로 넣어서 보내야 하는군요.. 공지사항 수정할 때 noticeId 값을 쿼리에 따로 전달하는 이유가 궁금했는데 궁금증이 해소되었습니다😄👍
fe-리뷰완
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.
fe-리뷰요청
frontend/src/api/notice.ts
Outdated
notices: NoticeResponse[]; | ||
} | ||
|
||
export type NoticeRequest = Omit<NoticeResponse, 'createdAt' | 'id'>; |
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.
공지사항 생성 시 id는 받지 않기 때문에 제외시켰습니당
{
title: string, (100자)
content: string, (3000자)
deadline: datetime (DHM 형태이다, yyyy-mm-dd HH:MM) : string
bannerTitle : string, (100자)
bannerSubtitle: string (100자)
}
return data; | ||
}, | ||
onError: () => { | ||
console.error('공지 사항을 리스트를 불러오는데 실패했습니다'); |
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.
알려주셔서 감사합니다 👍👍👍
frontend/src/types/notice.ts
Outdated
export interface NoticeList { | ||
totalPageNumber: number; | ||
currentPageNumber: number; | ||
noticeList: Notice[]; |
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.
제로 타입의 마술사이신가요? 🌊
그렇지만 바꾸지 않는 것이 더 유지 보수에 좋다고 생각이 들었습니다 그 이유는 아래와 같습니당
totalPageNumber와 currentPageNumber와 같은 명세가 백엔드에서 고쳐도 프론트에서는 고정이라고 생각을 하는데요. 유틸리티 타입으로 했을 때 오히려 수정하는 코드가 더 많아진다고 생각이 드는데 어떻게 생각하세요?
아래와 같이 api의 NoticeListResponse만 변경되어도 되는 코드가 제안해주신 코드로 한다면 프론트 코드에게 영향을 준다고 생각이 들어요
currentPageNumber가 pageNumber로 바뀌었을 경우 예시
export type NoticeList = Omit<Notice, 'notices' | 'pageNumber'> & { noticeList: Notice[] ; currentPageNumbre : number; };
제로! 제안해주신 코드에 대해 코멘트 달았읍니당 fe-리뷰요청 |
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.
우스! 정말 많은 테스트 코드를 작성하셨군요!
쿼리 훅이 많이 제작되었네요!
혹시 읽으면서 이미 머지된 imageZoomModal의 코드가 있는데,
이전 브랜치로 계속 작업하셔서 그런건가요..?
그 외에는 css 글로벌스타일에 대한 코멘트가 몇가지 있습니다만, approve 하겠습니다!
@@ -61,6 +61,14 @@ export default function CommentTextForm({ | |||
createComment({ content: deleteOverlappingNewLine(content) }); | |||
}; | |||
|
|||
const handleKeyboardCommentSubmit = (event: KeyboardEvent<HTMLTextAreaElement>) => { | |||
const isPressCtrlAndEnterKey = (event.metaKey || event.ctrlKey) && event.key === 'Enter'; |
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.
😆😆😆
@@ -20,12 +20,12 @@ export const TextArea = styled.textarea` | |||
resize: none; | |||
|
|||
&::placeholder { | |||
font-size: 14px; | |||
font-size: 1.4rem; |
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.
저희 얼른 1.4rem 글로벌 스타일 만들어야겠어요
} | ||
|
||
@media (max-width: ${theme.breakpoint.sm}) { | ||
&::placeholder { | ||
font-size: 12px; | ||
font-size: 1.2rem; |
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.
요건 var(--text-small)있는데 따로 처리 안하신 이유가 있나요?
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.
알려주셔서 감사합니다 ~ 👍
render: () => <ImageZoomModalStory />, | ||
}; | ||
|
||
function ImageZoomModalStory() { |
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.
요거 이전에 머지된 기능아닌가요..? 데자뷰..?
|
||
export const usePagedNoticeList = (initialPageNumber: number = 0) => { | ||
const [pageNumber, setPageNumber] = useState(initialPageNumber); | ||
const { data, isError, isLoading, error } = useQuery<NoticeList>( |
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.
오 이전에 이야기하실때 useInfiniteQuery 사용한다고 하셨던 것 같은데 useQuery를 사용하신 이유가 있으신가요??
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.
타입 관련 컨벤션을 페어프로그래밍을 통해 일치하였습니다!! 화이팅~~
🔥 연관 이슈
close: #737
📝 작업 요약
⏰ 소요 시간
2시간 45분 api가 많아서 단순 코드 작성만 하는데 생각보다 시간이 더 걸렸어요
🔎 작업 상세 설명
캐시타임, 스테일타임을 건드렸습니다
배너 공지 사항 : 30분 / 이유 : 배너 공지사항이 바뀌는 일이 적다고 생각하였지만 혹시 공지사항이 바뀌었을 때 캐싱되어 있어서 공지사항을 못보는 상황까지 고려해서 1시간은 길다고 생각했고, 30분으로 하였습니다. 기본값으로 하기엔 너무 안변하는 데이터라서요
공지 상세 정보 : 1시간 / 이유 : 공지사항 상세 정보는 더더욱 바뀔 일이 적다고 생각하여 1시간으로 설정했습니다
공지 목록 : 30분 / 이유 : 공지 사항 목록이 바뀌는 일이 적다고 생각했지만, 목록이다 보니 배너 공지사항이 바뀐다면 목록도 바뀌어야 하기에 배너 공지 사항 시간과 일치시켰습니다.
에러
현재 토스트 PR이 올라와있는 상태여서 우선 콘솔에러로 하였는데요. 추후 토스트 코드를 통해 에러나 성공을 보여주면 좋겠다고 생각했습니다
api 감싸기
api가 변하진 않았지만 한번 감싸주었는데요. 이렇게 하는 편이 나중에 쉽게 코드를 고칠 수 있을거라고 생각해서 감싸줘보았습니다