Skip to content

Conversation

@sookyeonghwang
Copy link
Member

작업 내용

  • 오늘 날짜로만 달력을 보여주도록 되어 있습니다.
  • 다른 달로 선택하면 그 달력을 보여주도록 구현하였습니다
  • API가 검색이 먼저 나올 것 같아 검색 먼저 구현하고 달력 마저 구현할 예정입니다
  • 디자인이 변경되기 전이기 때문에 코드 리팩토링 진행하고 검색 들어가겠습니다

참고 이미지(선택)

image

어떤 점을 리뷰 받고 싶으신가요?

@netlify
Copy link

netlify bot commented Feb 9, 2023

Deploy Preview for record-it-dev ready!

Name Link
🔨 Latest commit 5761113
🔍 Latest deploy log https://app.netlify.com/sites/record-it-dev/deploys/63e5272996c91c00084defb1
😎 Deploy Preview https://deploy-preview-210--record-it-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@Seongtaek-H Seongtaek-H left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확인했습니다.

Comment on lines +51 to +54
<DateBox date={1} gridColumnStart={monthYear.startDayOfMonth + 1} />
{[...Array(monthYear.lastDayOfMonth)].map((_, i) =>
i > 0 ? <DateBox key={i} date={i + 1} /> : null
)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1일 Datebox를 먼저 지정해주고 그 다음부터 순환하는 구조군요? 굿

return (
<div
className={`h-[36px] w-[36px] rounded-full text-[16px] font-medium text-grey-7`}
style={{ gridColumnStart }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 style을 넘버로 넣어주면 어떻게 되는거에요??

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style={{gridColumnStart: 4}} 이런식으로 들어가게 되는거에요!
속성 명과, props명이 같아서 축약해서 적었씁니다

setIsOpenCalendar: Dispatch<SetStateAction<boolean>>
}

export default function MyRecordCalendar({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이걸 밖으로 굳이 빼야되나 라는 생각이 듭니다
네이밍도 약간 헷갈리구요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분 워터가 추상화하신 이유가 있다면 현상유지 아니라면 저도 세인트 의견에 동감

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아아 이거 디자인이 변경되어서 이 부분 안쓸 것 같습니다 ㅎㅎㅎㅎㅎ,,,, ㅋㅋㅎㅋ
다음 작업 때 아예 없어지고 변경될 예정이에요!

Copy link
Contributor

@endmoseung endmoseung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

역시 워터코드는 수준이 높아 확인했습니다~

}: CalendarProps) {
const calendarRef = useClickOutside<HTMLDivElement>(() => {
setIsOpenCalendar(false)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 달력도 모달처럼 외부클릭하면 닫히나보네용 훅잘쓴거같아용!


if (isLoading) {
return <></>
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 저희 나중에 공통적으로 로딩 컴포넌트를 처리하면 좋을것같아요

<MemoryRecord />
</div>
{isOpenCalendar && (
<Calendar monthYear={monthYear} setIsOpenCalendar={setIsOpenCalendar} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 보니까 Calender에서는 setIsOpenCalendar를 true로 해주는 경우가 없더라고요.
생각해보면 캘린더 안에서는 캘린더를 끄는경우밖에 없으니 차라리 closeCalendar={()=>setIsOpenCalendar(false)}를 넘겨주는건 어떨까요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵넵 그렇게 해도 좋을 것 같아요

setIsOpenCalendar: Dispatch<SetStateAction<boolean>>
}

export default function MyRecordCalendar({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분 워터가 추상화하신 이유가 있다면 현상유지 아니라면 저도 세인트 의견에 동감

@sookyeonghwang sookyeonghwang merged commit 64e153c into develop Feb 10, 2023
@sookyeonghwang sookyeonghwang deleted the FE-193 branch February 10, 2023 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ Feature 기능 개발

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants