-
Notifications
You must be signed in to change notification settings - Fork 8
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]: 날짜를 선택할 수 있는 캘린더 UI 구현 #68
Conversation
- year, month를 상태로 관리하여 값이 변경될 때마다 날짜 정보를 계산해서 반환 - 달(Month)를 이동할 수 있는 함수 구현 - 12월에서 다음을 클릭할 경우 해(Year)가 변경되도록 구현
- 오늘의 경우 어떻게 보여줘야할 것인지 스타일 논의 필요 - 일요일이 아닌, 월요일부터 테이블을 채울 수 있도록 구현
- theme. global 스타일이 적용되도록 설정
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 16308cc. ♻️ This comment has been updated with latest results. |
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.
단기간에 굉장히 짜임새 있게 잘 구현하셨네요..
역시 해리 👍 잘 읽었고, 리뷰 몇 가지 남겼는데 확인 부탁드리겠습니다 :)
++ 현재 7월 24일인데 7월 1일~7월 23일이 선택되지 않게끔 설정해야할 것 같습니다! 이 점도 참고 부탁드려용!
export function getDayInfo( | ||
year: number, | ||
month: number, | ||
firstDayIndex: number, | ||
index: number, | ||
currentDate: Date, | ||
) { |
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.
[P3]
getDayInfo가 받는 인자가 꽤 많긴 하네요..! 혹시 의도된 부분일까요..?
아니라면 추후에 더 원활히 사용할 수 있도록 객체형태로 받아도 좋을 것 같아요!
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.
한 날짜와 관련된 정보들이 꽤 많았는데,
- 공휴일인지
- 선택된 날짜인지
- index를 사용하는 것에서 실제 날짜로 표시해야 하거나
- 날짜인지 아닌지 즉, 빈칸으로 표시해야하는 것인지 아닌지
- 오늘인지 아닌지
등등,,,
여러개의 함수로 분리해서 한 날짜와 관련된 정보를 구할 수 있겠지만, 날짜와 관련된 정보
라는 하나의 관심사로 보고 함수의 역할이 조금 많은 것 같더라도 한 함수에서 날짜와 관련된 정보를 구하도록 했습니다~ 그래서 인자도 자연스럽게 많아진 것 같아요. 인자가 많은 경우, 객체를 사용하지 않으면 인자 전달 순서를 모두 기억하고 있어야 하니 낙타 말 처럼, 객체 형태로 바꾸는 것이 좋을 것 같네요!
/* | ||
로직 설명(@해리) | ||
- 월요일을 index 0으로 변경하기 위해서 나머지 연산자를 활용한다. | ||
- 자바스크립트 데이트 객체는 기본적으로 일요일이 인덱스가 0인데, 모모 달력은 월요일을 인덱스를 0으로 만들어줘야 한다. | ||
- 따라서, 특정 달의 시작 날짜에 대한 인덱스에 6을 더해주고 7로 나눈 나머지를 사용하는 것으로 구현했다. | ||
*/ |
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.
로직 주석으로 설명을 해두니 이해하기 굉장히 편하네요..! 👍
min-width: 40px; | ||
height: 40px; | ||
min-height: 40px; |
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.
[P2]
여기 군데군데 px로 구현하신 부분이 보이는 것 같습니다..! rem으로 통일하는 것이 좋아보여서 리뷰 남깁니다 👍
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.
rem으로 통일하기로 했는데 완전히 잊고 있었네요..수정할게요!!
const { year, month, daySlotCount } = yearMonthInfo; | ||
|
||
return ( | ||
<div css={s_calendarContainer} aria-label={`${year}년 ${month}월 달력`}> |
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.
aria-label 명시.. 👍 대 해 리
<button key={dateString} onClick={() => onDateClick(dateString)} css={s_daySlotButton}> | ||
<span css={[s_daySlot(isHoliday), s_selectedDaySlot(isSelectedDate)]}> | ||
{isDate ? date : ''} | ||
</span> | ||
</button> |
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.
[P3]
날짜 선택을 할 수 없는 첫 줄의 빈 공간에도 button이 삽입되는데, 웹 표준에 맞게 구현하기 위해서는 버튼이 없도록 만들어야 할 것 같아요! 급한 것은 아니니 추후에 수정해도 괜찮구요!
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.
oh...모든 요소들을 다 버튼으로 하니 날짜가 아닌 경우에도 cursor : pointer
속성이 적용되고 있었네요...! 이 부분은 접근성을 위해서라도 빠르게 수정할게요!!
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.
border: none; | ||
`; | ||
|
||
// TODO : 공휴일 색 변경 논의 필요(@해리) |
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.
익스텐션 뭐죵!?
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.
@hwinkr 혹시 자동완성 익스텐션이 있어야 가능한가요? 지금 해리는 안 되나요?
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 endDate = new Date(year, month, 0); | ||
const lastDate = endDate.getDate(); |
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.
[P2]
endDate
와 lastDate
변수명은 의미가 유사하여 쉽게 파악하기 어려운 것 같습니다.
const lastDayOfMonthDate = new Date(year, month, 0); // 해당 월의 마지막 날짜 객체
const lastDayNumber = lastDayOfMonthDate.getDate(); // 마지막 날짜의 일(day) 값
이런 느낌으로 좀 더 네이밍을 명확하게 바꿔보면 어떨까요?
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.
다시 보니까, 이해하기 힘든 것 같긴 하군요...빙봉 추천대로 수정할게요~
스토리북에서도 전역 스타일을 적용할 수 있도록 했었는데 (abf8706) 다크모드가 적용된 것이 아니라, 눈 고통 이슈로 모모 앱 배경 색을 어두운 회색으로 했었는데, 해당 배경색이 적용된 것 같아요..ㅎㅋㅋ |
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.
사항 반영 야무지게 해주셨네요 감사합니다
👍👍👍 💯💯💯 🐫🐫🐫
@@ -8,7 +8,7 @@ export const s_calendarContainer = css` | |||
|
|||
export const s_calendarContent = css` | |||
display: grid; | |||
grid-auto-rows: 40px; | |||
grid-auto-rows: 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.
👍👍👍 💯💯💯
</button> | ||
) : ( | ||
<div key={dateString} css={s_daySlotButton}></div> |
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.
👍👍👍 💯💯💯
관련 이슈
작업 내용
>
버튼을 클릭해서 다음 달로 이동할 수 있다,<
를 클릭해서 이전 달로 돌아갈 수 있다.특이 사항
컴포넌트 사용법
아래 props들을 주입한다.
=>
useCalendar
과 같은 훅으로 분리할 수 있다.2차 스프린트가 끝난 후 개선해야 하는 사항들
Date
객체로는 한국의 공휴일을 판단할 수 없음.aria-label
적극 활용리뷰 요구사항 (선택)