-
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
공지사항 목록 관리자 페이지 구현 / 공지사항 생성, 수정, 삭제 기능 #780
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.
코멘트 달았습니다!!!
고생하셨어요!!
이제 시간이 얼마 남지 않았는데, 우스랑 함께 코드를 짤 수 있어서 너무 좋았습니다!
우리 조금 더 힘내봐요!!
|
||
interface AdminNoticeWriteProps { | ||
notice: Notice; | ||
writeType: '수정' | '작성'; |
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.
아래와 같이 상수화하여 타입 지정했습니다
타입은 따로 파일을 만들지 않았는데요
다른 곳에서 사용하는 곳이 없고, export 했을 때 자동완성 목록에서 안 나오면 좋겠어서 컴포넌트 내부에 타입 선언해주었습니다
export const NOTICE_WRITE_TYPE = {
CREATE: '작성',
EDIT: '수정',
} as const;
---------------------
type NoticeWriteType = (typeof NOTICE_WRITE_TYPE)[keyof typeof NOTICE_WRITE_TYPE];
interface AdminNoticeWriteProps {
notice: Notice;
writeType: NoticeWriteType;
writeNotice: (notice: NoticeRequest) => void;
}
export default function AdminNoticeWrite({
notice,
rows?: number; | ||
} | ||
|
||
function CustomTextarea({ label, handleTextChange, text, limit, rows = 10 }: CustomInputProps) { |
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.
공지사항 작성에서만 사용할 목적으로 Textarea를 만들었습니다 따로 export 하기에는 범용성이 적다고 생각했어요
스타일도 많이 적용을 해서 분리하기엔 애매하다고 생각이 드는데 어떻게 생각하시나여? 🌊
return ( | ||
<S.TextareaContainer> | ||
<S.Label htmlFor={label}> | ||
{label} ({text.length}/{limit}) |
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/routes/router.tsx
Outdated
children: [ | ||
{ | ||
path: PATH.NOTICES.slice(1), | ||
path: `${PATH.REPORT.slice(1)}/pending`, |
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.
다른 것들을 보니 children은 그냥 path로 작성햇네요..
이야기해서 통일이 필요할 것 같아요.
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.
네 좋아요! 의논해서 리팩터링하게 되더라도 다른 이슈에서 하도록 통일성있게 해놓겠습니다 👍👍👍
const newMonth = String(newTime.getMonth() + 1).padStart(2, '0'); | ||
const newHour = String(newTime.getHours()).padStart(2, '0'); | ||
const newMinute = String(newTime.getMinutes()).padStart(2, '0'); | ||
const newDay = +String(newTime.getDate()).padStart(2, '0'); |
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.
이렇게 하면 잘 나오나요..?
이럼 숫자 9 > 문자 09 > 숫자 9 이렇게 되지 않아요??
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.
갓우스🚀
변수명 관련된 코멘트가 좀 있어요!
항상 테스트 주도 개발하시는 모습 넘 멋져요👍💯
[23, 20, [21, 22, 23]], | ||
[2, 0, [1, 2]], | ||
[10, 3, [1, 2, 3, 4, 5]], | ||
])( |
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/components/notice/AdminNoticeTableFetcher/index.tsx
Outdated
Show resolved
Hide resolved
editButton: ( | ||
<S.EditButtonWrapper to={`${PATH.ADMIN_NOTICE}/${id}`}> | ||
<SquareButton theme="fill">수정하기</SquareButton> | ||
</S.EditButtonWrapper> |
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.
너무 좋습니당
저번에 라이트하우스 하면서 보기=>보러가기 했던것이 너무 인상깊었나봐요 ㅋㅋㅋ
columns={[ | ||
'제목', | ||
'내용', | ||
'배너 타이틀', |
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.
setPage(item); | ||
}} | ||
> | ||
<SquareButton theme={page === item - 1 ? 'fill' : 'blank'}>{item}</SquareButton> |
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.
최애 common 컴포넌트입니다
} | ||
|
||
export default function AdminNoticeWrite({ | ||
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.
혹시 PostForm 처럼 어떤 컴포넌트인지 직관적으로 와닿게 NoticeForm 어떠신가요?
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.
통일성 있어서 좋네요!
* @param size 페이지를 몇개씩 보여줄 것인지의 단위 default: 5 | ||
* @returns | ||
*/ | ||
export const usePagination = (initialPageNumber: number = 0, size: number = 5) => { |
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.
오 hook으로 페이징 로직을 분리해주셨군요!
저는 url 값으로 페이징해서 다음 페이지 가면 페이지 전체를 렌더링하는데.. 우스처럼 hook 을 쓰면 목록 Table 상태만 갈아끼울 수 있다는 장점이 있네요! 저도 이 방식으로 리팩터링해봐야겠어요😃
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.
사용자를 동료 개발자를 타겟으로 하여 페이지 기능을 만들 때 범용성 있게 사용되도록 할 목적으로 만들었습니다 🌊
url로 페이징하면 페이지 전체를 렌더링 하는 것은 처음 알게 되었네요 !
frontend/src/hooks/usePagination.ts
Outdated
setPageNumber(value - 1); | ||
}; | ||
|
||
const isCheckNextPage = (totalPage: 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.
isNextPage 나 checkNextPage 어떠신가요?
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.
boolean을 반환하는 함수 이름 명은 너무 어렵네요
isNextPage는 함수라는 생각이 안들어서요 checkNextPage가 적절해보여서 수정해보겠습니다
근데 다시 생각해보니 isCheck~~는 체크했나 확인하는 같은 느낌이 들어서 함수명이라는 생각이 확 들지 않네요
|
||
import * as S from './style'; | ||
|
||
export default function NoticeAdminPage() { |
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.
admin 경로로 옮기는거 어떠신가용
pages/admin/notices 이렇게요!
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.
다른 폴더들은 notice와 같이 s를 안 붙혔는데 admin/notice/~~Page가 있을 것을 생각하니 s를 붙히는 것이 더 적절해보보여서 notices로 했습니당
fe-리뷰완 |
코드 리뷰 주셨던 부분들을 코드 수정하여 개선해보았습니다 👍👍 fe-리뷰요청 |
🔥 연관 이슈
close: #759
📝 작업 요약
⏰ 소요 시간
5시간
🔎 작업 상세 설명
공지사항 목록
삭제하기 버튼을 눌렀을 떄
공지사항 생성/ 수정 페이지
달력은 input으로 구현하였습니다 그리고 현재 시간 이전으로는 선택 불가능하도록 하였고, 모든 input에는 required 속성을 추가해주었습니다
그리고 현재 시간을 StringDate 형식으로 사용하기 위해서 utils/post 함수를 수정했습니다