Skip to content

Conversation

@endmoseung
Copy link
Contributor

작업 내용

-중복 사용됨에 따라 Card컴포넌트화
-TogetherSlider에서 Card컴포넌트로 대체

참고 이미지(선택)

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

free하게 남겨주세용~

-중복 사용됨에 따라 Card컴포넌트화
-TogetherSlider에서 Card컴포넌트로 대체
@endmoseung endmoseung added the ✨ Feature 기능 개발 label Feb 10, 2023
@endmoseung endmoseung self-assigned this Feb 10, 2023
@netlify
Copy link

netlify bot commented Feb 10, 2023

Deploy Preview for record-it-dev ready!

Name Link
🔨 Latest commit cbbda22
🔍 Latest deploy log https://app.netlify.com/sites/record-it-dev/deploys/63e5f6f1ceefed000701096f
😎 Deploy Preview https://deploy-preview-213--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.

확인했습니다

const navigate = useNavigate()

const handleClickRecord = (recordId: number) => {
if (isDragging && setIsDragging) {
Copy link
Member

Choose a reason for hiding this comment

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

setIsDragging 은 왜들어가죠?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이부분 세인트가 원래 작성한부분에서 dragging을 체크하는데 저는 그부분이 필요없어서 ?연산자로 받고 세인트코드가 작동하도록 했어요

Copy link
Member

Choose a reason for hiding this comment

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

그럼 setIsDragging을 뺴고 isDragging 으로만 하는게 더 좋을것같습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 그러면 이게 setIsDragging이 ?연산자라서 없을경우도 있기떄문에 저걸 넣었어요!

</div>
isDragging={isDragging}
setIsDragging={setIsDragging}
item={item}
Copy link
Member

Choose a reason for hiding this comment

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

item 전체를 그냥 내려버리는거보다 Item.colorName, item.iconName 을 내려주는게 더 명확해보입니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그래야겠네요 이렇게 하면 item이라는 객체에 종속돼서 확장성이 떨어질거같아요

Copy link
Member

@sookyeonghwang sookyeonghwang 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 +20 to +27
interface RecordCategory {
recordId: number
title: string
iconName: string
iconColor: string
}

export interface IMemoryRecord extends RecordCategory {
Copy link
Member

Choose a reason for hiding this comment

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

좋아욤

import { useCheckMobile } from '@hooks/useCheckMobile'
import { useNavigate } from 'react-router-dom'

interface CardProps {
Copy link
Member

Choose a reason for hiding this comment

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

이름은 Card보다는 RecordCard 처럼 구체적이면 훨씬 좋을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이부분 고민했었는데 그게 더 명확해보이네용 확인입니당~

-컴포넌트명 수정
-의존성 수정
@endmoseung endmoseung merged commit d1d2b23 into develop Feb 10, 2023
@endmoseung endmoseung deleted the FE-214 branch February 17, 2023 01:49
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